-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI: dom usage refactoring #4924
Changes from all commits
78a163d
1b9a994
00663a2
3a81b37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,8 @@ import Grid from 'ember-collection/layouts/grid'; | |
import SlotsMixin from 'ember-block-slots'; | ||
import WithResizing from 'consul-ui/mixins/with-resizing'; | ||
import style from 'ember-computed-style'; | ||
import qsaFactory from 'consul-ui/utils/dom/qsa-factory'; | ||
import sibling from 'consul-ui/utils/dom/sibling'; | ||
import closest from 'consul-ui/utils/dom/closest'; | ||
import clickFirstAnchorFactory from 'consul-ui/utils/dom/click-first-anchor'; | ||
const clickFirstAnchor = clickFirstAnchorFactory(closest); | ||
|
||
import { inject as service } from '@ember/service'; | ||
import { computed, get, set } from '@ember/object'; | ||
/** | ||
* Heavily extended `ember-collection` component | ||
|
@@ -24,8 +20,6 @@ import { computed, get, set } from '@ember/object'; | |
* in the future | ||
*/ | ||
|
||
// ember doesn't like you using `$` hence `$$` | ||
const $$ = qsaFactory(); | ||
// need to copy Cell in wholesale as there is no way to import it | ||
// there is no change made to `Cell` here, its only here as its | ||
// private in `ember-collection` | ||
|
@@ -85,9 +79,10 @@ const change = function(e) { | |
// 'actions_close' would mean that all menus have been closed | ||
// therefore we don't need to calculate | ||
if (e.currentTarget.getAttribute('id') !== 'actions_close') { | ||
const $tr = closest('tr', e.currentTarget); | ||
const $group = sibling(e.currentTarget, 'ul'); | ||
const $footer = [...$$('footer[role="contentinfo"]')][0]; | ||
const dom = get(this, 'dom'); | ||
const $tr = dom.closest('tr', e.currentTarget); | ||
const $group = dom.sibling(e.currentTarget, 'ul'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yuk, how did I ever think this was a good idea! 😄 Actually I need to have a better look and refresh my memory on these utils, maybe there was a reason, but I doubt it! |
||
const $footer = dom.element('footer[role="contentinfo"]'); | ||
const groupRect = $group.getBoundingClientRect(); | ||
const footerRect = $footer.getBoundingClientRect(); | ||
const groupBottom = groupRect.top + $group.clientHeight; | ||
|
@@ -122,6 +117,7 @@ export default Component.extend(SlotsMixin, WithResizing, { | |
style: style('getStyle'), | ||
checked: null, | ||
hasCaption: false, | ||
dom: service('dom'), | ||
init: function() { | ||
this._super(...arguments); | ||
this.change = change.bind(this); | ||
|
@@ -136,11 +132,12 @@ export default Component.extend(SlotsMixin, WithResizing, { | |
}), | ||
resize: function(e) { | ||
const $tbody = this.element; | ||
const $appContent = [...$$('main > div')][0]; | ||
const dom = get(this, 'dom'); | ||
const $appContent = dom.element('main > div'); | ||
if ($appContent) { | ||
const border = 1; | ||
const rect = $tbody.getBoundingClientRect(); | ||
const $footer = [...$$('footer[role="contentinfo"]')][0]; | ||
const $footer = dom.element('footer[role="contentinfo"]'); | ||
const space = rect.top + $footer.clientHeight + border; | ||
const height = e.detail.height - space; | ||
this.set('height', Math.max(0, height)); | ||
|
@@ -273,7 +270,7 @@ export default Component.extend(SlotsMixin, WithResizing, { | |
}, | ||
actions: { | ||
click: function(e) { | ||
return clickFirstAnchor(e); | ||
return get(this, 'dom').clickFirstAnchor(e); | ||
}, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ export default Controller.extend(WithFiltering, { | |
}; | ||
}); | ||
}), | ||
// TODO: This should be using a searchable | ||
filter: function(item, { s = '', type = '' }) { | ||
const sLower = s.toLowerCase(); | ||
return ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spotted I hadn't used the new searchables in the old acls system, actually I've just thought it will have been because it was old and I was changing as little as possible with the new ACL upgrade, possibly the 2nd PR to come in this series. |
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my first question. Now I have all these things in the
dom
service, it would make sense to getwindow
from there also. I was thinking of making aviewport
method, which would be a nice word whether window was a browser window, an iframe (not really a window) like when ember testing, or anything else. To me the viewport is what we are actually talking about here.I'm like 90% sure I want to do this, thought it might be useful to ask what others think?