UI: dom usage refactoring #4924

johncowen · 2018-11-08T16:25:28Z

This PR is possibly the first in a series of refactoring PR's to use some features written during my work on the new ACLs system, and also a general spring clean before getting on with my next tranche of feature work.

I was going to put everything in a single PR, but the things I want to do are growing (specifically on some nice recommendations from @DingoEatingFuzz ) so figured it would be nicer to split into separate PRs.

Refactoring DOM utility usage throughout the app

I had previously begun moving all of my dom related utilities into a single dom Service. Previously they were simply imported when needed, and then setup when needed. Moving them into a service means I can reduce this repetitive set up to a single place and the utilities are ready to be used as an when I need them.

There are a few utilities which might cause a bit of confusion as to why they are in a dom service, mostly around the event utilities. I did wonder about this myself, but I found that Event/EventTarget things are officially DOM interfaces and therefore I figure they do actually belong in a 'dom' service.

https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model#DOM_interfaces

The work here just changes areas that used the previously imported the utilities to use the newer dom service. It also adds a new components utility which is just a variation on the previous component utility.

I have a few decisions to make which would be good to get some opinion on before doing, I'll try and add these queries inline.

This includes an additional `dom.components` method which gives you a list of components matching the selector instead of just one.

johncowen · 2018-11-08T16:29:41Z

ui-v2/app/components/hashicorp-consul.js

 export default Component.extend({
+  dom: service('dom'),
+  // TODO: could this be dom.viewport() ?
+  win: window,
  isDropdownVisible: false,


This is my first question. Now I have all these things in the dom service, it would make sense to get window from there also. I was thinking of making a viewport 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?

johncowen · 2018-11-08T16:31:45Z

ui-v2/app/components/tabular-collection.js

-        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');


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!

johncowen · 2018-11-08T16:33:11Z

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 (


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.

johncowen · 2018-11-08T16:36:42Z

ui-v2/app/services/dom.js


 // 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() {


Along the same idea as viewport above I'm thinking of exposing document as just dom.document(), I'm not referring to document that much in my codebase but I may aswell go the whole hog here?

johncowen · 2018-11-08T16:40:04Z

Sorry also a couple of things I couldn't do inline:

I'm thinking of removing some comments also, mainly due to decisions that have been made, or me thinking things are clear enough:

consul/ui-v2/app/services/dom.js

Lines 39 to 47 in 3a81b37

    
           // TODO: Should I change these to use the standard names 
        
           // even though they don't have a standard signature (querySelector*) 
        
           elementById: function(id) { 
        
             return get(this, 'doc').getElementById(id); 
        
           }, 
        
           elementsByTagName: function(name, context) { 
        
             context = typeof context === 'undefined' ? get(this, 'doc') : context; 
        
             return context.getElementByTagName(name); 
        
           },

I'm happy with these names I think, I like that they aren't the standard names as they don't necessarily follow the same signature.

consul/ui-v2/app/services/dom.js

Lines 60 to 69 in 3a81b37

    
           // ember components aren't strictly 'dom-like' 
        
           // but if you think of them as a web component 'shim' 
        
           // then it makes more sense to think of them as part of the dom 
        
           // with traditional/standard web components you wouldn't actually need this 
        
           // method as you could just get to their methods from the dom element 
        
           component: function(selector, context) { 
        
             // TODO: support passing a dom element, when we need to do that 
        
             return $_(this.element(selector, context)); 
        
           }, 
        
           components: function(selector, context) {

Thinking of removing the big non-TODO comment here, I'm happy that this belongs here, but happy to leave it in if anyone thinks it helps.

`window` and `document` are easily injected anyhow, but this primarily this keeps everything dom related in the same place. Included here are changes to make all ember related objects use the dom service `document` and `viewport` instead of just `document` and `window`. Quote from a previous PR (#4924) which explains the thinking around this: > Now I have all these things in the dom service, it would make sense to get window from there also. I was thinking of making a viewport 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.

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.