Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to new modifier apis (modify) instead of lifecycles #57

Open
wants to merge 3 commits into
base: gen-4
Choose a base branch
from

Conversation

betocantu93
Copy link
Contributor

@betocantu93 betocantu93 commented Oct 22, 2023

ember-modifers deprecates lifecycles methods, such as didInstall, didUpdate... replaced by modify.

here are some migration guides

https://github.com/ember-modifier/ember-modifier/blob/main/MIGRATIONS.md

ember-modifers deprecates lifecycles methods, such as didInstall, didUpdate... replaced by modify.

here are some migration guides

https://github.com/ember-modifier/ember-modifier/blob/main/MIGRATIONS.md
Copy link
Owner

@lolmaus lolmaus left a comment

Choose a reason for hiding this comment

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

Wow, thanks! This is awesome!

Please fix two very minor comments and add a CHANGELOG.md entry (in this project it's maintained by hand).

Note: Node version bump is a breaking change, so bump the major version in the changelog.

@@ -41,9 +43,11 @@ export default class ElementQueryModifier extends Modifier<ModifierArgs> {
sizesHeightDefault: Sizes = SIZES_HEIGHT_DEFAULT;
sizesRatioDefault: Sizes = SIZES_RATIO_DEFAULT;

_element?: HTMLElement; // For some reason, this.element is not always available
_element?: HTMLElement; // For some reason, this._element is not always available
Copy link
Owner

Choose a reason for hiding this comment

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

This is wrong, please revert.


this.didResizeHandler();
registerDestructor(this, () => this.cleanup());
Copy link
Owner

Choose a reason for hiding this comment

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

Why this change (arrow function)? In order to remove the lint exception? But we have this exact lint exception used all over the file, so I don't see a point in fixing just one instance.

The linter should serve to the developer, not vice versa. :)

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.

None yet

2 participants