From c532e53d9a7688e9d7a4a664074c9d6cdd5479f9 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Mon, 19 Nov 2018 14:47:20 +0000 Subject: [PATCH] UI: dom usage refactoring (#4924) Move all the dom-things to use the dom service in tabular-collection, feedback-dialog, list-collection and node show. Move get-component-factory into utils/dom and use dom.root() in a few more places This includes an additional `dom.components` method which gives you a list of components matching the selector instead of just one. --- ui-v2/app/components/app-view.js | 15 +++++---- ui-v2/app/components/feedback-dialog.js | 9 ++++-- ui-v2/app/components/hashicorp-consul.js | 21 ++++++++---- ui-v2/app/components/list-collection.js | 15 +++++---- ui-v2/app/components/tabular-collection.js | 23 ++++++------- ui-v2/app/components/tabular-details.js | 8 ++--- ui-v2/app/controllers/dc/acls/index.js | 1 + ui-v2/app/controllers/dc/nodes/show.js | 21 ++++++------ ui-v2/app/controllers/dc/services/show.js | 3 +- ui-v2/app/routes/application.js | 20 ++++++------ ui-v2/app/services/dom.js | 32 ++++++++++++++++--- .../utils/{ => dom}/get-component-factory.js | 0 ui-v2/tests/unit/routes/application-test.js | 8 +---- .../{ => dom}/get-component-factory-test.js | 4 +-- 14 files changed, 103 insertions(+), 77 deletions(-) rename ui-v2/app/utils/{ => dom}/get-component-factory.js (100%) rename ui-v2/tests/unit/utils/{ => dom}/get-component-factory-test.js (90%) diff --git a/ui-v2/app/components/app-view.js b/ui-v2/app/components/app-view.js index eaf2f029fe..ed526eab0d 100644 --- a/ui-v2/app/components/app-view.js +++ b/ui-v2/app/components/app-view.js @@ -1,31 +1,33 @@ import Component from '@ember/component'; import SlotsMixin from 'block-slots'; import { get } from '@ember/object'; +import { inject as service } from '@ember/service'; import templatize from 'consul-ui/utils/templatize'; -const $html = document.documentElement; export default Component.extend(SlotsMixin, { loading: false, authorized: true, enabled: true, classNames: ['app-view'], classNameBindings: ['enabled::disabled', 'authorized::unauthorized'], + dom: service('dom'), didReceiveAttrs: function() { // right now only manually added classes are hoisted to + const $root = get(this, 'dom').root(); let cls = get(this, 'class') || ''; if (get(this, 'loading')) { cls += ' loading'; } else { - $html.classList.remove(...templatize(['loading'])); + $root.classList.remove(...templatize(['loading'])); } if (cls) { // its possible for 'layout' templates to change after insert // check for these specific layouts and clear them out - [...$html.classList].forEach(function(item, i) { + [...$root.classList].forEach(function(item, i) { if (templatize(['edit', 'show', 'list']).indexOf(item) !== -1) { - $html.classList.remove(item); + $root.classList.remove(item); } }); - $html.classList.add(...templatize(cls.split(' '))); + $root.classList.add(...templatize(cls.split(' '))); } }, didInsertElement: function() { @@ -34,7 +36,8 @@ export default Component.extend(SlotsMixin, { didDestroyElement: function() { const cls = get(this, 'class') + ' loading'; if (cls) { - $html.classList.remove(...templatize(cls.split(' '))); + const $root = get(this, 'dom').root(); + $root.classList.remove(...templatize(cls.split(' '))); } }, }); diff --git a/ui-v2/app/components/feedback-dialog.js b/ui-v2/app/components/feedback-dialog.js index ebdc003fce..b42df6304e 100644 --- a/ui-v2/app/components/feedback-dialog.js +++ b/ui-v2/app/components/feedback-dialog.js @@ -1,8 +1,7 @@ import Component from '@ember/component'; import { get, set } from '@ember/object'; import { inject as service } from '@ember/service'; -import qsaFactory from 'consul-ui/utils/dom/qsa-factory'; -const $$ = qsaFactory(); +import { Promise } from 'rsvp'; import SlotsMixin from 'block-slots'; const STATE_READY = 'ready'; @@ -10,6 +9,7 @@ const STATE_SUCCESS = 'success'; const STATE_ERROR = 'error'; export default Component.extend(SlotsMixin, { wait: service('timeout'), + dom: service('dom'), classNames: ['with-feedback'], transition: '', transitionClassName: 'feedback-dialog-out', @@ -23,6 +23,7 @@ export default Component.extend(SlotsMixin, { applyTransition: function() { const wait = get(this, 'wait').execute; const className = get(this, 'transitionClassName'); + // TODO: Make 0 default in wait wait(0) .then(() => { set(this, 'transition', className); @@ -30,7 +31,9 @@ export default Component.extend(SlotsMixin, { }) .then(() => { return new Promise(resolve => { - $$(`.${className}`, this.element)[0].addEventListener('transitionend', resolve); + get(this, 'dom') + .element(`.${className}`, this.element) + .addEventListener('transitionend', resolve); }); }) .then(() => { diff --git a/ui-v2/app/components/hashicorp-consul.js b/ui-v2/app/components/hashicorp-consul.js index 69272e5b16..93a3064bc9 100644 --- a/ui-v2/app/components/hashicorp-consul.js +++ b/ui-v2/app/components/hashicorp-consul.js @@ -1,11 +1,15 @@ import Component from '@ember/component'; import { get, set } from '@ember/object'; -const $html = document.documentElement; -const $body = document.body; +import { inject as service } from '@ember/service'; export default Component.extend({ + dom: service('dom'), + // TODO: could this be dom.viewport() ? + win: window, isDropdownVisible: false, didInsertElement: function() { - $html.classList.remove('template-with-vertical-menu'); + get(this, 'dom') + .root() + .classList.remove('template-with-vertical-menu'); }, actions: { dropdown: function(e) { @@ -14,12 +18,15 @@ export default Component.extend({ } }, change: function(e) { + const dom = get(this, 'dom'); + const $root = dom.root(); + const $body = dom.element('body'); if (e.target.checked) { - $html.classList.add('template-with-vertical-menu'); - $body.style.height = $html.style.height = window.innerHeight + 'px'; + $root.classList.add('template-with-vertical-menu'); + $body.style.height = $root.style.height = get(this, 'win').innerHeight + 'px'; } else { - $html.classList.remove('template-with-vertical-menu'); - $body.style.height = $html.style.height = null; + $root.classList.remove('template-with-vertical-menu'); + $body.style.height = $root.style.height = null; } }, }, diff --git a/ui-v2/app/components/list-collection.js b/ui-v2/app/components/list-collection.js index f3f41f2a28..1a5229583e 100644 --- a/ui-v2/app/components/list-collection.js +++ b/ui-v2/app/components/list-collection.js @@ -1,11 +1,12 @@ +import { inject as service } from '@ember/service'; import { computed, get, set } from '@ember/object'; import Component from 'ember-collection/components/ember-collection'; import PercentageColumns from 'ember-collection/layouts/percentage-columns'; import style from 'ember-computed-style'; import WithResizing from 'consul-ui/mixins/with-resizing'; -import qsaFactory from 'consul-ui/utils/dom/qsa-factory'; -const $$ = qsaFactory(); + export default Component.extend(WithResizing, { + dom: service('dom'), tagName: 'div', attributeBindings: ['style'], height: 500, @@ -30,11 +31,13 @@ export default Component.extend(WithResizing, { }; }), resize: function(e) { - const $self = this.element; - const $appContent = [...$$('main > div')][0]; + // TODO: This top part is very similar to resize in tabular-collection + // see if it make sense to DRY out + const dom = get(this, 'dom'); + const $appContent = dom.element('main > div'); if ($appContent) { - const rect = $self.getBoundingClientRect(); - const $footer = [...$$('footer[role="contentinfo"]')][0]; + const rect = this.element.getBoundingClientRect(); + const $footer = dom.element('footer[role="contentinfo"]'); const space = rect.top + $footer.clientHeight; const height = e.detail.height - space; this.set('height', Math.max(0, height)); diff --git a/ui-v2/app/components/tabular-collection.js b/ui-v2/app/components/tabular-collection.js index 15241374f3..3705703344 100644 --- a/ui-v2/app/components/tabular-collection.js +++ b/ui-v2/app/components/tabular-collection.js @@ -5,12 +5,8 @@ import Grid from 'ember-collection/layouts/grid'; import SlotsMixin from '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'); + 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); }, }, }); diff --git a/ui-v2/app/components/tabular-details.js b/ui-v2/app/components/tabular-details.js index 8b1d2d0fb6..fd569b8608 100644 --- a/ui-v2/app/components/tabular-details.js +++ b/ui-v2/app/components/tabular-details.js @@ -1,14 +1,14 @@ import Component from '@ember/component'; import SlotsMixin from 'block-slots'; -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 { get } from '@ember/object'; export default Component.extend(SlotsMixin, { + dom: service('dom'), onchange: function() {}, actions: { click: function(e) { - clickFirstAnchor(e); + get(this, 'dom').clickFirstAnchor(e); }, change: function(item, e) { this.onchange(e, item); diff --git a/ui-v2/app/controllers/dc/acls/index.js b/ui-v2/app/controllers/dc/acls/index.js index 4da546da81..3a28d5a76d 100644 --- a/ui-v2/app/controllers/dc/acls/index.js +++ b/ui-v2/app/controllers/dc/acls/index.js @@ -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 ( diff --git a/ui-v2/app/controllers/dc/nodes/show.js b/ui-v2/app/controllers/dc/nodes/show.js index 8c900df163..f90de09113 100644 --- a/ui-v2/app/controllers/dc/nodes/show.js +++ b/ui-v2/app/controllers/dc/nodes/show.js @@ -1,12 +1,9 @@ import Controller from '@ember/controller'; +import { inject as service } from '@ember/service'; import { get, set, computed } from '@ember/object'; -import { getOwner } from '@ember/application'; import WithSearching from 'consul-ui/mixins/with-searching'; -import qsaFactory from 'consul-ui/utils/dom/qsa-factory'; -import getComponentFactory from 'consul-ui/utils/get-component-factory'; - -const $$ = qsaFactory(); export default Controller.extend(WithSearching, { + dom: service('dom'), queryParams: { s: { as: 'filter', @@ -36,15 +33,15 @@ export default Controller.extend(WithSearching, { actions: { change: function(e) { set(this, 'selectedTab', e.target.value); - const getComponent = getComponentFactory(getOwner(this)); // Ensure tabular-collections sizing is recalculated // now it is visible in the DOM - [...$$('.tab-section input[type="radio"]:checked + div table')].forEach(function(item) { - const component = getComponent(item); - if (component && typeof component.didAppear === 'function') { - getComponent(item).didAppear(); - } - }); + get(this, 'dom') + .components('.tab-section input[type="radio"]:checked + div table') + .forEach(function(item) { + if (typeof item.didAppear === 'function') { + item.didAppear(); + } + }); }, sortChecksByImportance: function(a, b) { const statusA = get(a, 'Status'); diff --git a/ui-v2/app/controllers/dc/services/show.js b/ui-v2/app/controllers/dc/services/show.js index fd3153661f..d4653888a5 100644 --- a/ui-v2/app/controllers/dc/services/show.js +++ b/ui-v2/app/controllers/dc/services/show.js @@ -1,6 +1,5 @@ import Controller from '@ember/controller'; -import { get } from '@ember/object'; -import { computed } from '@ember/object'; +import { get, computed } from '@ember/object'; import sumOfUnhealthy from 'consul-ui/utils/sumOfUnhealthy'; import hasStatus from 'consul-ui/utils/hasStatus'; import WithHealthFiltering from 'consul-ui/mixins/with-health-filtering'; diff --git a/ui-v2/app/routes/application.js b/ui-v2/app/routes/application.js index 3b85e9cfba..7645d51c75 100644 --- a/ui-v2/app/routes/application.js +++ b/ui-v2/app/routes/application.js @@ -3,13 +3,11 @@ import { inject as service } from '@ember/service'; import { hash } from 'rsvp'; import { get } from '@ember/object'; import { next } from '@ember/runloop'; -import { Promise } from 'rsvp'; -import WithBlockingActions from 'consul-ui/mixins/with-blocking-actions'; -const $html = document.documentElement; -const removeLoading = function() { - return $html.classList.remove('ember-loading'); +const removeLoading = function($from) { + return $from.classList.remove('ember-loading'); }; -export default Route.extend(WithBlockingActions, { +export default Route.extend({ + dom: service('dom'), init: function() { this._super(...arguments); }, @@ -17,20 +15,21 @@ export default Route.extend(WithBlockingActions, { settings: service('settings'), actions: { loading: function(transition, originRoute) { + const $root = get(this, 'dom').root(); let dc = null; if (originRoute.routeName !== 'dc') { const model = this.modelFor('dc') || { dcs: null, dc: { Name: null } }; dc = get(this, 'repo').getActive(model.dc.Name, model.dcs); } hash({ - loading: !$html.classList.contains('ember-loading'), + loading: !$root.classList.contains('ember-loading'), dc: dc, }).then(model => { next(() => { const controller = this.controllerFor('application'); controller.setProperties(model); transition.promise.finally(function() { - removeLoading(); + removeLoading($root); controller.setProperties({ loading: false, dc: model.dc, @@ -74,6 +73,7 @@ export default Route.extend(WithBlockingActions, { if (error.status === '') { error.message = 'Error'; } + const $root = get(this, 'dom').root(); hash({ error: error, dc: @@ -85,13 +85,13 @@ export default Route.extend(WithBlockingActions, { dcs: model && model.dcs ? model.dcs : [], }) .then(model => { - removeLoading(); + removeLoading($root); next(() => { this.controllerFor('error').setProperties(model); }); }) .catch(e => { - removeLoading(); + removeLoading($root); next(() => { this.controllerFor('error').setProperties({ error: error }); }); diff --git a/ui-v2/app/services/dom.js b/ui-v2/app/services/dom.js index 6b84f21035..ae88e0f5a0 100644 --- a/ui-v2/app/services/dom.js +++ b/ui-v2/app/services/dom.js @@ -2,25 +2,36 @@ import Service from '@ember/service'; import { getOwner } from '@ember/application'; import { get } from '@ember/object'; +// selecting import qsaFactory from 'consul-ui/utils/dom/qsa-factory'; -// TODO: Move to utils/dom -import getComponentFactory from 'consul-ui/utils/get-component-factory'; +// TODO: sibling and closest seem to have 'PHP-like' guess the order arguments +// ie. one `string, element` and the other has `element, string` +// see if its possible to standardize +import sibling from 'consul-ui/utils/dom/sibling'; +import closest from 'consul-ui/utils/dom/closest'; +import getComponentFactory from 'consul-ui/utils/dom/get-component-factory'; + +// events import normalizeEvent from 'consul-ui/utils/dom/normalize-event'; import createListeners from 'consul-ui/utils/dom/create-listeners'; +import clickFirstAnchorFactory from 'consul-ui/utils/dom/click-first-anchor'; // ember-eslint doesn't like you using a single $ so use double // use $_ for components const $$ = qsaFactory(); let $_; +const clickFirstAnchor = clickFirstAnchorFactory(closest); export default Service.extend({ doc: document, init: function() { this._super(...arguments); $_ = getComponentFactory(getOwner(this)); }, - normalizeEvent: function() { - return normalizeEvent(...arguments); - }, + // TODO: should this be here? Needs a better name at least + clickFirstAnchor: clickFirstAnchor, + closest: closest, + sibling: sibling, + normalizeEvent: normalizeEvent, listeners: createListeners, root: function() { return get(this, 'doc').documentElement; @@ -35,6 +46,8 @@ export default Service.extend({ return context.getElementByTagName(name); }, elements: function(selector, context) { + // don't ever be tempted to [...$$()] here + // it should return a NodeList return $$(selector, context); }, element: function(selector, context) { @@ -53,4 +66,13 @@ export default Service.extend({ // TODO: support passing a dom element, when we need to do that return $_(this.element(selector, context)); }, + components: function(selector, context) { + return [...this.elements(selector, context)] + .map(function(item) { + return $_(item); + }) + .filter(function(item) { + return item != null; + }); + }, }); diff --git a/ui-v2/app/utils/get-component-factory.js b/ui-v2/app/utils/dom/get-component-factory.js similarity index 100% rename from ui-v2/app/utils/get-component-factory.js rename to ui-v2/app/utils/dom/get-component-factory.js diff --git a/ui-v2/tests/unit/routes/application-test.js b/ui-v2/tests/unit/routes/application-test.js index 5474f3d76c..e5bf49c9d4 100644 --- a/ui-v2/tests/unit/routes/application-test.js +++ b/ui-v2/tests/unit/routes/application-test.js @@ -2,13 +2,7 @@ import { moduleFor, test } from 'ember-qunit'; moduleFor('route:application', 'Unit | Route | application', { // Specify the other units that are required for this test. - needs: [ - 'service:repository/dc', - 'service:settings', - 'service:feedback', - 'service:flashMessages', - 'service:logger', - ], + needs: ['service:repository/dc', 'service:settings', 'service:dom'], }); test('it exists', function(assert) { diff --git a/ui-v2/tests/unit/utils/get-component-factory-test.js b/ui-v2/tests/unit/utils/dom/get-component-factory-test.js similarity index 90% rename from ui-v2/tests/unit/utils/get-component-factory-test.js rename to ui-v2/tests/unit/utils/dom/get-component-factory-test.js index 16be5c677e..bbac456045 100644 --- a/ui-v2/tests/unit/utils/get-component-factory-test.js +++ b/ui-v2/tests/unit/utils/dom/get-component-factory-test.js @@ -1,7 +1,7 @@ -import getComponentFactory from 'consul-ui/utils/get-component-factory'; +import getComponentFactory from 'consul-ui/utils/dom/get-component-factory'; import { module, test } from 'qunit'; -module('Unit | Utility | get component factory'); +module('Unit | Utility | dom/get component factory'); test("it uses lookup to locate the instance of the component based on the DOM element's id", function(assert) { const expected = 'name';