[gaia-components] Add support for DOM event delegation to view.js

NEW
Unassigned

Status

Firefox OS
Gaia::Components
4 years ago
a year ago

People

(Reporter: justindarc, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Our 'View' component (currently only used by Camera) will likely be re-used in other apps in the near future. Currently, it is extremely loosely-defined as far as how to attach event listeners in the DOM. It also provides no mechanism for handling event delegation. For example:

In Camera, views/hud.js:

  View.extend({
    name: 'hud',

    initialize: function() {
      this.render();
    },

    render: function() {
      this.el.innerHTML = this.template();
      this.els.flash = this.find('.js-flash');
      this.els.camera = this.find('.js-camera');
      this.els.settings = this.find('.js-settings');
      bind(this.els.flash, 'click', this.onFlashClick);
      bind(this.els.camera, 'click', this.onCameraClick);
      bind(this.els.settings, 'click', this.onSettingsClick, true);
    },
    ...
  });

Here, we have to query the DOM 3 times to get 3 references to each of the buttons in the 'view'. Then, we have to individually attach event listeners to each of the 3 buttons. It would be preferable from a performance and a code cleanliness standpoint if we could follow a similar pattern to Backbone's 'View' class:

  View.extend({
    name: 'hud',

    events: {
      'click .hud_btn': 'onButtonClick'
    },

    onButtonClick: function(evt) {
      // Get the name of the action to emit from the
      // `data-action` attribute on the button:
      // <div class="hud_btn" data-action="settings"></div>
      this.emit('click:' + evt.target.dataset.action);
    },
    ...
  });

Also, by defining a standard way of attaching DOM event listeners in our views, we could provide a reasonable default implementation of `initialize` and `render` that would not need to be manually overridden for most View subclasses.
(Reporter)

Comment 1

4 years ago
Created attachment 8460542 [details] [review]
pull-request (master)

Patch for adding built-in support for DOM event delegation.
Assignee: nobody → jdarcangelo
Attachment #8460542 - Flags: review?(wilsonpage)
Attachment #8460542 - Flags: review?(dmarcos)
Comment on attachment 8460542 [details] [review]
pull-request (master)

As mentioned on Github I think this logic should be in a separate module. We already have the 'Attach' module that is used in places throughout Camera. This module is unit-tested and good to go. Perhaps we can make this a dependency of View, or establish a standard way of using it it with View?
Attachment #8460542 - Flags: review?(wilsonpage) → review-
(Reporter)

Comment 3

4 years ago
Comment on attachment 8460542 [details] [review]
pull-request (master)

Unflagging for review. After talking to Wilson, we are going to simply `require` the attach.js lib from gaia-components into the base View class so it can remain tested separately. Will re-flag for review once my changes are complete.
Attachment #8460542 - Flags: review?(dmarcos)
Attachment #8460542 - Flags: review-
(Reporter)

Updated

4 years ago
Component: Gaia → Gaia::Components
(Reporter)

Comment 4

4 years ago
Comment on attachment 8460542 [details] [review]
pull-request (master)

Before we do another review, I just wanted to get some feedback from both of you to see if this is looking ok. All event delegation is now handled by attach.js which helps lighten the amount of code in our base View class. I also added an example HTML page and set up unit tests, but haven't built any real tests yet. If this looks ok to you guys, that will be the next thing I add before final review.
Attachment #8460542 - Flags: feedback?(wilsonpage)
Attachment #8460542 - Flags: feedback?(dmarcos)
Comment on attachment 8460542 [details] [review]
pull-request (master)

Looks good man!

- Would be good to wack in few more tests, especially for the new code paths added.
- Wondering if we could improve the Attach API to make this stuff a little easier. What if something like:

// On initialize...
this.attach = new Attach(this.el);
this.detach = this.attach.off.bind(this.attach);
this.resolveEvents(); // replace strings with real functions
this.attach(this.events);

Just some ideas, no reason it has to be done now.
Attachment #8460542 - Flags: feedback?(wilsonpage) → feedback+
(Reporter)

Comment 6

4 years ago
Comment on attachment 8460542 [details] [review]
pull-request (master)

Wilson: I've updated the PR and want you to take another look. I've included the enhancement you did to the ControlsView in Camera where we use CSS classes in addition to `data-` attributes for get() and set(). However, in doing so, I noticed that it seemed that we had a lot of redundant things in our base View class. For instance, we had a show() and hide() that effectively could do the same thing as enable() and disable(). I'd prefer that we had some consistency in how we perform common tasks like this, so I removed show() and hide() and re-wrote the toggle() method to alternate between enable() and disable() instead. I've updated the included example to utilize the new CSS class-based enable()/disable()/toggle() so you can see it in action.

Also, I will be adding more tests before I submit this for final review. I just want to make sure we nail down the API through these periodic feedbacks before I go ahead and write more tests.
Attachment #8460542 - Flags: feedback+ → feedback?(wilsonpage)
Comment on attachment 8460542 [details] [review]
pull-request (master)

(In reply to Justin D'Arcangelo [:justindarc] from comment #6)
> Comment on attachment 8460542 [details] [review]
> pull-request (master)
> 
> Wilson: I've updated the PR and want you to take another look. I've included
> the enhancement you did to the ControlsView in Camera where we use CSS
> classes in addition to `data-` attributes for get() and set(). However, in
> doing so, I noticed that it seemed that we had a lot of redundant things in
> our base View class. For instance, we had a show() and hide() that
> effectively could do the same thing as enable() and disable(). I'd prefer
> that we had some consistency in how we perform common tasks like this, so I
> removed show() and hide() and re-wrote the toggle() method to alternate
> between enable() and disable() instead. I've updated the included example to
> utilize the new CSS class-based enable()/disable()/toggle() so you can see
> it in action.

Cool, nice one! I'd argue that we still need show/hide API as well as enable/disable. I see disabling a view (such as hud) as making it 'unusable' (like with pointer-events: none). But hiding the view makes it visually hidden. I know we use both techniques throughout Camera.

Both methods simply add/remove semantic CSS classes to the view's root element, and it's up to the view's style-sheet to add the necessary style properties. It's important that the base class doesn't make these style assumptions.

Some other smaller issues in Github comments.
Attachment #8460542 - Flags: feedback?(wilsonpage) → feedback-
(Reporter)

Comment 8

2 years ago
Not working on this. Unassigning myself.
Assignee: jdarcangelo → nobody
(Reporter)

Updated

a year ago
Attachment #8460542 - Flags: feedback?(dmarcos)
You need to log in before you can comment on or make changes to this bug.