Added MDC Tab bar scroller by danielraggs · Pull Request #21 · secondstreet/ember-material-components-web

danielraggs · 2017-08-01T16:03:51Z

Material Design Tab Bar Scroller component is now available for use in Ember Material Components Web. This component acts as a parent container for mdc-tab-bar by invoking an instance of it inside its Handlebars.

Example implementation:

{{#mdc-tab-bar-scroller as |scroller|}}
    {{#scroller.tab-bar links=false as |bar|}}
      {{#bar.tab}}
        Red
      {{/bar.tab}}
      {{#bar.tab}}
        Yellow
      {{/bar.tab}}
      {{#bar.tab}}
        Green
      {{/bar.tab}}
      {{#bar.tab}}
        White
      {{/bar.tab}}
      {{#bar.tab}}
        Blue
      {{/bar.tab}}
    {{/scroller.tab-bar}}
{{/mdc-tab-bar-scroller}}

Kerrick · 2017-08-01T16:11:46Z


 export default Ember.Component.extend(MDCComponent, {
  //region Attributes
+  /**


What does the @private signify, given that it's within Attributes which are usually public?

The attribute scrolling only ever gets used inside mdc-tab-bar-scroller.hbs, where it gets passed into {{mdc-tab-bar}} as true. In that sense yes it's an attribute, but I marked it as private because this isn't something the user should ever be passing on their own into the {{mdc-tab-bar}}, it's handled internally by the code.

Kerrick · 2017-08-01T16:12:35Z

  },
+  didInsertElement() {
+    this._super(...arguments);
+    if (get(this, 'scrolling')) {


It seems like you might want to gate this based on the presence of the action instead of scrolling.

Yeah that's a good idea. If I do this then I no longer need the scrolling attribute also.

Kerrick · 2017-08-01T16:12:41Z

+  },
+  willDestroyElement() {
+    this._super(...arguments);
+    if (get(this, 'scrolling')) {


It seems like you might want to gate this based on the presence of the action instead of scrolling.

Kerrick · 2017-08-24T19:42:12Z

+  },
+  didInsertElement() {
+    this._super(...arguments);
+    set(this, 'scrollFrameElement', getElementProperty(this, 'querySelector')(strings.FRAME_SELECTOR));


Please be sure to have a fallback for getElementProperty when it's supposed to return a function. Example:

getElementProperty(this, 'querySelector', () => null)(strings.FRAME_SELECTOR)

Kerrick · 2017-08-24T19:44:24Z

+  CLASS_NAMES: cssClasses,
+
+  /**
+   * @type {Object}


I believe these should be @property not @type, and should be HTMLElement instead of Object.

I can make this change but just to double-check, was this also supposed to apply to mdc-menu.js as well? We have a property there that uses @type {HTMLElement} instead of @property.

Whoops! Those should eventually be fixed, but they shouldn't block this PR.

Kerrick · 2017-08-24T19:47:46Z

+      removeClassFromForwardIndicator: (className) => run(() => get(this, 'forwardIndicatorClasses').removeObject(className)),
+      addClassToBackIndicator: (className) => run(() => get(this, 'backIndicatorClasses').addObject(className)),
+      removeClassFromBackIndicator: (className) => run(() => get(this, 'backIndicatorClasses').removeObject(className)),
+      isRTL: () => getElementProperty(this, 'direction') === 'rtl',


I believe you want to get dir, not direction. Alternatively, you can do getComputedStyle for the CSS direction.

Kerrick · 2017-08-24T19:54:12Z

+      addClassToBackIndicator: (className) => run(() => get(this, 'backIndicatorClasses').addObject(className)),
+      removeClassFromBackIndicator: (className) => run(() => get(this, 'backIndicatorClasses').removeObject(className)),
+      isRTL: () => getElementProperty(this, 'direction') === 'rtl',
+      registerBackIndicatorClickHandler: (handler) => get(this, 'backIndicatorElement').addEventListener('click', handler),


I'm not sure if this will happen or not, but I have a concern:

If the back indicator element is un-rendered and re-rendered as a different DOM element by Ember after the click handler was registered, then it will no longer have a click event listener attached. This is bad, and should be avoided.

If this is a problem, I see two ways to avoid it:

Store the back indicator element click handlers in the component JS, and re-attach them every time on didRender

Store the back indicator element click handlers in the component JS, and use the click Ember hook for the whole component, firing off each back indicator element click handler if the event's target was the back indicator element.

I think I'd prefer the latter of those two, and I'm not sure that list is exhaustive.

This applies to the other register/deregisterHandler methods that apply listeners directly to the elements, too.

I'm not sure I understand-- why would the back indicator element ever be un-rendered without the entire component itself being un-rendered?

You're completely right, it wouldn't be. This was only a risk if there was a boolean around the indicators or something, which there's clearly not.

Kerrick · 2017-08-28T15:24:01Z

+      removeClassFromForwardIndicator: (className) => run(() => get(this, 'forwardIndicatorClasses').removeObject(className)),
+      addClassToBackIndicator: (className) => run(() => get(this, 'backIndicatorClasses').addObject(className)),
+      removeClassFromBackIndicator: (className) => run(() => get(this, 'backIndicatorClasses').removeObject(className)),
+      isRTL: () => getComputedStyle(get(this, 'element')).getPropertyValue('direction') === 'rtl',


It seems like this might run the risk of this.element being missing because the component has been destroyed -- probably safest to change it to something like:

!!this && !get(this, 'isDestroyed') && !!get(this, 'element') && getComputedStyle(get(this, 'element')).getPropertyValue('direction') === 'rtl'

Kerrick · 2017-08-28T15:26:56Z

+  },
+  willDestroyElement() {
+    this._super(...arguments);
+    if (get(this, 'deregister-tab-bar')) {


You won't have to wrap this or the other in an if if you add a default for deregister-tab-bar in the Attributes region.

Kerrick · 2017-08-28T16:54:58Z

Thank you so much for the PR! This will ship out soon with v0.0.35 👍

- 🐛 Fix unsafe HTML element access - ✨ Dark theme can be used for tab bars and tab bar scrollers (#26) - ⚡ Don't unnecessarily (de|re)-attach handlers (#25) - ✨ Implement tab-bar-scroller from @material/tabs (#21) - ✨ Implement @material/switch (#24)

Daniel Raggs added 2 commits July 28, 2017 14:06

Upgrade MDC tabs and prepare dummy app for tab bar scroller

ea9fcb0

✨ Implement mdc-tab-bar-scroller

a7d6929

Kerrick reviewed Aug 1, 2017

View reviewed changes

Daniel Raggs added 2 commits August 1, 2017 12:05

➕ The addon now installs material-design-icons

4301536

🔨 Avoid underscored property names; remove unneeded properties

8313280

Kerrick reviewed Aug 24, 2017

View reviewed changes

secondstreet deleted a comment from danielraggs Aug 24, 2017

👌 Various code review changes

b2fd810

Kerrick approved these changes Aug 28, 2017

View reviewed changes

👌 More code review changes

468036f

Kerrick merged commit daf9f3e into master Aug 28, 2017

Kerrick deleted the tab-bar-scroller branch October 12, 2017 20:56

Conversation

danielraggs commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kerrick Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kerrick commented Aug 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danielraggs commented Aug 1, 2017 •

edited

Loading

Kerrick Aug 24, 2017 •

edited

Loading