Skip to content

Conversation

Gaurav0
Copy link

@Gaurav0 Gaurav0 commented Nov 29, 2021

No description provided.

Copy link

@danielraggs danielraggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left you just a few questions. Most important one concerns the proper way to refactor/handle the classNameBindings and attributeBindings.

Comment on lines +48 to +57
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]),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm noticing this change show up for many other files too so my question applies to those instances as well.

Copy link
Author

@Gaurav0 Gaurav0 Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +83 to +90
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)),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really how the linter prefers functions formatted now? Strange, the move away from this in the past seemed fine to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this version of Ember not yet expect this to instead be this.scrollFrameElement?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do think this.scrollFrameElement will work, I haven't made this change generally.

DrVonDevious
DrVonDevious previously approved these changes Dec 1, 2021
danielraggs
danielraggs previously approved these changes Dec 1, 2021
Copy link
Contributor

@Kerrick Kerrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ember 6.0?

@Gaurav0 Gaurav0 dismissed stale reviews from danielraggs and DrVonDevious via 1288313 December 2, 2021 15:07
Copy link
Contributor

@Kerrick Kerrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving my review section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants