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

Visibility: onOnScreen doesn't fire if element is onScreen on first render #1911

Closed
lafritay opened this issue Jul 30, 2017 · 5 comments
Closed
Assignees

Comments

@lafritay
Copy link
Contributor

Steps

The goal here is to create an element that has an image that only shows when the image is visible.

  1. Create an element with an image.
  2. Wrap the image with a visibility element that fires onOnScreen.
  3. When the image first draws, set its src to ''
  4. When the image is on screen, set its src to the desired image so it downloads.

Expected Result

  1. Images that are rendered on screen (above the fold) on the first render have onOnScreen to set their source to the desired image.
  2. Images that are rendered off screen are only loaded once the page is scrolled to a point where the image is on the screen.

Actual Result

Images that are rendered on screen (above the fold) on the first render do not fire onOnScreen.

Version

0.69.0

Testcase

https://codepen.io/anon/pen/MvKPJX?editors=0010

Note, I just used icons in this example. The example says that the github icon should be 'users' when it is off screen and 'github' when it is onscreen. Notice that on initial load it is 'users'. If you scroll the window ever so slightly, it'll change to 'github'.

If there is another way I should be accomplishing this first render case, please let me know.

@layershifter layershifter changed the title Visibility onOnScreen doesn't fire if element is onScreen on first render Visibility: onOnScreen doesn't fire if element is onScreen on first render Jul 31, 2017
@levithomason
Copy link
Member

levithomason commented Aug 11, 2017

There are a couple options for moving forward here. The current props related to this are:

continuous - on scroll, fire callbacks anytime the callback conditions are met
once - on scroll, fire callbacks each time a callback becomes true

You can try the SUI core examples for these here.

However, the intent of the behavior detection is to fire on scroll only. There is currently no prop such as initial, which would fire the callbacks once immediately.

I think I would be OK adding an initial prop for this case. If true, we would fire callbacks componentDidMount and also componentWillReceiveProps if the value changed from false to true.

Thoughts?

@lafritay
Copy link
Contributor Author

That seems very reasonable. The only question I'd have is what would happen in the following scenario:

  1. Both initial and once were passed in.
  2. The element starts on the screen, hence, the events are fired
  3. A scroll happens.

Do the events fire again for the scroll related change? My assumption is that they wouldn't. In other words - once fires events once they become true, regardless of which action they became true because of. Thoughts?

@levithomason
Copy link
Member

once + intial

Good question. I think the intuitive thing is to keep the behavior of once restricted to calling the callbacks a total of 1 time for the life of the component, it is "once" after.

The initial prop then would change when the callbacks are called once. Are they called once on the first scroll event that triggers them (default), or are they called once on mount (i.e. initial)?

Naming change?

The only other thing that bugs me here is initial is a new naming convention, most of the other props in our library are based on the name *OnMount:

  • unmountOnHide, mountOnShow, transitionOnMount (Transition)
  • onUnmount / onMount (Portal, Popup, Modal)

This makes me think we should probably go with the React lifecyle inspired prop name rather than invent a new SUI prop name. What about:

  • fireOnMount
  • callbackOnMount
  • other ideas...

Feedback?

@lafritay
Copy link
Contributor Author

Yes, I agree. fireOnMount is much more intuitive as well. I like it.

@levithomason
Copy link
Member

Settled, PRs welcome 👍

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

No branches or pull requests

3 participants