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

New prop added - fullView #254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

byungjune
Copy link

if the waypoint is displayed on the screen fully, not partly.
The onEneter/onLeave fire

if the waypoint is displayed on the screen fully, not partly.
The onEneter/onLeave fire
@MatthewHerbst
Copy link
Contributor

Hi @onyoon7, thanks for the PR, and sorry for the delay in responding to it. @trotzig any objections to merging this?

@jamesplease
Copy link
Collaborator

jamesplease commented Jun 7, 2018

Hi there! Thanks for the PR, @onyoon7 !

I think it could be useful if you could provide more information about the use case for this functionality. Before we merge it in I think we should have a better understanding of the use case it solves. My gut feeling says that this use case can probably be solved by just using a regular Waypoint (without children).

Separately, I have a few thoughts about this implementation:

By default Waypoints have no dimensions; they are one dimensional lines. It's only in situations where you provide children that they have height along the scroll axis, so I think it would be valuable to make it more clear that this only applies in that situation. We don't want people to get the wrong impression about what a Waypoint is 🙂

When it comes to children, this library has some opinions. From the docs:

The onEnter callback will be called when any part of the child is visible in the viewport. The onLeave callback will be called when all of the child has exited the viewport.

My initial read of this PR is that it is providing the ability to change onEnter to wait for all of the children to appear. If that's the case, then a more complete API would make to allow the onLeave behavior to be toggled as well.

I think it's worth considering if it's possible to accomplish whatever the motivation for this PR is without any API changes, though ✌️

Thanks for reading, and thanks again for the PR!

@byungjune
Copy link
Author

Thank you for reading about this Pull request and reviewing it in detail @jamesplease . 🙇‍♂️

Here are some reasons I suggested fullView:

  • Run a callback if the child is completely visible
    For example, when a customer saw a product on the eCommerce site, I realized that the customer was aware of the product and wanted to run a callback. In this case, onEnter and onLeave do not know if the entire child has been viewed.

  • onEnter: executed when child starts to be visible

  • fullView: run when the entire child is visible

  • onLeave: run when child starts to be hidden

If you feel that the above features are unnecessary or have any good improvements, please feel free to do so.

Thank you for reading.

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.

3 participants