-
Notifications
You must be signed in to change notification settings - Fork 92
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
feat(modifiers): add in-viewport
, did-enter-viewport
, did-exit-viewport
#208
Conversation
Why the |
Quoting from #205 (comment):
This assertion checks that you only ever use on modifier on a given element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks phenomenal. Really appreciate this addition. I think if we can simplify around in-viewport
modifier with optional onEnter and onExit callbacks, I think this will be a great addition for the community.
} | ||
} | ||
|
||
didReceiveArguments() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you happen to know how this is functionally different than didReceiveAttrs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API docs for ember-class-based-modifier
explain this quite well:
didReceiveArguments()
Called when the modifier is installed and anytime the arguments are updated.
So AFAICT they are functionally equivalent.
package.json
Outdated
@@ -22,7 +22,9 @@ | |||
"license": "MIT", | |||
"dependencies": { | |||
"ember-auto-import": "^1.2.15", | |||
"ember-class-based-modifier": "^0.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we have the same support matrix. 👍
"ember-cli-babel": "^7.7.3", | ||
"fast-deep-equal": "^2.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Going to pull this into intersection-observer-admin
|
||
name = 'in-viewport'; | ||
|
||
lastOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work? Just evaluates to undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Uninitialized class instance properties are undefined
, but the property slot is already created. I add these out out of habit (TypeScript FTW), but supposedly this also improves performance because of shaping. Not sure about that one though. The inner workings of JS engines are beyond my horizon. 🤷♂ 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All for defining ALL THE SHAPES!
@buschtoens 👋 Just following up on this PR! Ignore me if you are just waiting for a sliver of time to follow up. Otherwise, just wanted to make sure you saw this:
|
Yes 🙈
I didn't indeed! So you think we can drop the additional |
heya! what happened here? is this going forward? I have some components that I want to make template only / tagless and this would be great to have! |
584c2a3
to
a5c1a9c
Compare
Left to do IIRC:
|
a5c1a9c
to
383e176
Compare
Docs-wise I'd honestly recommend to do a complete revamp regarding the usage and deprecate the In the scope of this PR I would completely replace the docs for Modifiers with the built-in modifiers and slightly reword Classes to put focus on the direct service usage. The complete docs revamp is out of scope, for this PR IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be fine with merging this and then I can help re-work the docs a bit and add tests, eventually bumping a minor version. Then follow up with a major version removing the use of the Mixin....
Does that sound ok with you @buschtoens? Also, mind swapping out the yarn lock file with the package-lock file?
Sorry, that I didn't get to reply yet, but thanks for merging! The tests are definitely broken / not finished and the docs are missing, but I'm not sure whether I got time to PR them anytime soon. 🙊 |
Closes #205.
/cc @mehulkar @snewcomer
Adds the following modifiers:
{{in-viewport}}
: acceptsonEnter
/onExit
and further named watcher options.{{did-enter-viewport}}
: accepts a positional argument as the callback and further named watcher options.{{did-exit-viewport}}
: accepts a positional argument as the callback and further named watcher options.Exemplary usage is best seen in
infinity-built-in-modifiers.hbs
:Passing watcher options would look like this:
This PR is yet missing tests and docs.