Conversation
Update Changelog collapse versions
danielraggs
left a comment
danielraggs
left a comment
There was a problem hiding this comment.
Left you just a few questions. Most important one concerns the proper way to refactor/handle the classNameBindings and attributeBindings.
| classNameBindings: Object.freeze([ | ||
| 'secondary:mdc-button--accent', | ||
| 'raised:mdc-button--raised', | ||
| 'unelevated:mdc-button--unelevated', | ||
| 'compact:mdc-button--compact', | ||
| 'dense:mdc-button--dense', | ||
| 'stroked:mdc-button--stroked', | ||
| 'mdcClassNames', | ||
| ], | ||
| attributeBindings: ['disabled', 'type', 'style', ...events], | ||
| ]), | ||
| attributeBindings: Object.freeze(['disabled', 'type', 'style', ...events]), |
There was a problem hiding this comment.
Was this a robot/automated change? I would think the ideal way to refactor classNameBindings and attributeBindings would be to turn this into a Glimmer component and move the logic to the handlebars.
There was a problem hiding this comment.
I'm noticing this change show up for many other files too so my question applies to those instances as well.
There was a problem hiding this comment.
I wanted to make a minimal change to pass lint rather than refactoring to glimmer components, which may affect compatibility. This was not done by a codemod, but was an automated search/replace.
| addClass: (className) => next(() => addClass(className, this)), | ||
| removeClass: (className) => next(() => removeClass(className, this)), | ||
| registerAnimationEndHandler: (handler) => | ||
| getElementProperty(this, 'addEventListener', () => null)(ANIM_END_EVENT_NAME, handler), | ||
| deregisterAnimationEndHandler: handler => | ||
| deregisterAnimationEndHandler: (handler) => | ||
| getElementProperty(this, 'removeEventListener', () => null)(ANIM_END_EVENT_NAME, handler), | ||
| registerChangeHandler: handler => run(() => get(this, 'changeHandlers').addObject(handler)), | ||
| deregisterChangeHandler: handler => run(() => get(this, 'changeHandlers').removeObject(handler)), | ||
| registerChangeHandler: (handler) => run(() => get(this, 'changeHandlers').addObject(handler)), | ||
| deregisterChangeHandler: (handler) => run(() => get(this, 'changeHandlers').removeObject(handler)), |
There was a problem hiding this comment.
Is this really how the linter prefers functions formatted now? Strange, the move away from this in the past seemed fine to me.
There was a problem hiding this comment.
This is one of the prettier 2.x changes.
| getScrollLeftForScrollFrame: () => get(this, 'scrollFrameElement').scrollLeft, | ||
| setScrollLeftForScrollFrame: scrollLeftAmount => (get(this, 'scrollFrameElement').scrollLeft = scrollLeftAmount), | ||
| setScrollLeftForScrollFrame: (scrollLeftAmount) => | ||
| (get(this, 'scrollFrameElement').scrollLeft = scrollLeftAmount), |
There was a problem hiding this comment.
Does this version of Ember not yet expect this to instead be this.scrollFrameElement?
There was a problem hiding this comment.
While I do think this.scrollFrameElement will work, I haven't made this change generally.
Kerrick
left a comment
Kerrick
left a comment
There was a problem hiding this comment.
Review for files not in addon/ or tests/integration/components/
package.json
Outdated
| "demoURL": "https://secondstreet.github.io/ember-material-components-web/", | ||
| "versionCompatibility": { | ||
| "ember": ">=2.10.0" | ||
| "ember": ">=6.0" |
Gaurav0
dismissed stale reviews from danielraggs and calebrutland
via
1288313
Kerrick
left a comment
Kerrick
left a comment
There was a problem hiding this comment.
Approving my review section
4 participants
No description provided.