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

feat(modifiers): add in-viewport, did-enter-viewport, did-exit-viewport #208

Merged
merged 4 commits into from
Dec 16, 2019

Conversation

buschtoens
Copy link
Contributor

Closes #205.
/cc @mehulkar @snewcomer

Adds the following modifiers:

  • {{in-viewport}}: accepts onEnter / 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:

{{#if (eq this.direction "both")}}
  <ul>
    {{#each this.models as |artwork i|}}
      <li
        {{in-viewport
          onEnter=(action "didEnterViewport" artwork i)
          onExit=(action "didExitViewport" artwork i)
        }}
      >
        {{dummy-artwork artwork=artwork artworkProfile="dummy"}}
      </li>
    {{/each}}
  </ul>
{{else if (eq this.direction "enter")}}
  <ul>
    {{#each this.models as |artwork i|}}
      <li {{did-enter-viewport (action "didEnterViewport" artwork i)}}>
        {{dummy-artwork artwork=artwork artworkProfile="dummy"}}
      </li>
    {{/each}}
  </ul>
{{else if (eq this.direction "exit")}}
  <ul>
    {{#each this.models as |artwork i|}}
      <li {{did-exit-viewport (action "didExitViewport" artwork i)}}>
        {{dummy-artwork artwork=artwork artworkProfile="dummy"}}
      </li>
    {{/each}}
  </ul>
{{/if}} 

Passing watcher options would look like this:

<li
  {{in-viewport
    onEnter=(action "didEnterViewport" artwork i)
    onExit=(action "didExitViewport" artwork i)
    viewportTolerance=(hash top=100)
 }}
>
  {{dummy-artwork artwork=artwork artworkProfile="dummy"}}
</li>

<li
  {{did-enter-viewport
    (action "didEnterViewport" artwork i)
    viewportTolerance=(hash top=100)
 }}
>
  {{dummy-artwork artwork=artwork artworkProfile="dummy"}}
</li>

This PR is yet missing tests and docs.

@mehulkar
Copy link
Contributor

Why the in-viewport modifier? Why introduce 2 ways to do the same thing?

@buschtoens
Copy link
Contributor Author

buschtoens commented Sep 26, 2019

Quoting from #205 (comment):

One issue I found while spiking out an implementation is that watchElement accepts configOptions:

Options as the second argument to inViewport.watchElement include intersectionThreshold, scrollableArea, viewportSpy && viewportTolerance.

I'd add these as named args to the modifiers, like so:

<div
  {{did-enter-viewport this.didEnterViewport viewportTolerance=100}}
>
</div>

This is an issue, when you want to use both modifiers at the same time and may want to have different configs:

<div
  {{did-enter-viewport this.didEnterViewport viewportTolerance=100}}
  {{did-exit-viewport  this.didExitViewport  viewportTolerance=50}}
>
</div>

I did not deep dive into the source code yet, but maybe someone already knows whether we could support this, with a slight refactor maybe?

This assertion checks that you only ever use on modifier on a given element.

Copy link
Collaborator

@snewcomer snewcomer left a 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() {
Copy link
Collaborator

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?

Copy link
Contributor Author

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",
Copy link
Collaborator

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",
Copy link
Collaborator

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;
Copy link
Collaborator

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?

Copy link
Contributor Author

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. 🤷‍♂ 😄

Copy link
Collaborator

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!

@snewcomer
Copy link
Collaborator

@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:

I think if we can simplify around in-viewport modifier with optional onEnter and onExit callbacks...

@buschtoens
Copy link
Contributor Author

if you are just waiting for a sliver of time to follow up

Yes 🙈

just wanted to make sure you saw this:

I think if we can simplify around in-viewport modifier with optional onEnter and onExit callbacks...

I didn't indeed! So you think we can drop the additional {{did-enter-viewport}} / {{did-exit-viewport}} in favor of just {{in-viewport}}?

@mehulkar
Copy link
Contributor

mehulkar commented Dec 5, 2019

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!

@buschtoens
Copy link
Contributor Author

buschtoens commented Dec 5, 2019

Left to do IIRC:

  • remove {{did-enter-viewport}} / {{did-exit-viewport}} in favor of just {{in-viewport}}
  • write tests
  • write docs

@buschtoens
Copy link
Contributor Author

Docs-wise I'd honestly recommend to do a complete revamp regarding the usage and deprecate the InViewportMixin and Ember object model entirely. Instead the officially recommended usage should be the built-in modifiers. Besides it the only other official public API should be the direct interaction with the in-viewport service.

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.

Copy link
Collaborator

@snewcomer snewcomer left a 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?

@snewcomer snewcomer merged commit bd0ba9b into DockYard:master Dec 16, 2019
@buschtoens
Copy link
Contributor Author

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. 🙊

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

Successfully merging this pull request may close these issues.

Modifier without mixin
3 participants