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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(use-viewport-scroll): support passing element ref #195

Merged
merged 2 commits into from
May 15, 2020
Merged

feat(use-viewport-scroll): support passing element ref #195

merged 2 commits into from
May 15, 2020

Conversation

souporserious
Copy link
Contributor

This was a quick take on adding ref support to useViewportScroll, curious your thoughts on everything. Still new to Typescript, so I'm sure I'm missing some things 馃槆. Please comment on any concerns and I can address them. I can also update documentation once there's some finalization and take a stab at adding tests.

Some questions I had while implementing this:

  • Should we probably warn if ref.current is available but is not overflow: scroll?
  • Another solution could be to just grab the scroll closest scroll container, I have a small function from another library I can add to implement this
  • We should probably handle only adding one listener per element/window similar to what you had originally
  • Can we utilize useResize hook and implement ResizeObserver inside that?

I understand if you don't want to add ResizeObserver support. Although, I think it could be ok since most modern browsers will support this in their next release. We could also add docs and point to the polyfill since older browser support is probably needed.

Closes #182

@mattgperry
Copy link
Collaborator

Hey man, thanks for taking a look at this! To answer your questions:

Should we probably warn if ref.current is available but is not overflow: scroll?

I don't think this is necessary tbh. If the element doesn't scroll there probably won't be a broken expectation when scroll values don't update.

Another solution could be to just grab the scroll closest scroll container, I have a small function from another library I can add to implement this

I don't think we should be too permissive in this instance.

We should probably handle only adding one listener per element/window similar to what you had originally

Yeah so I definitely would like window to only have one listener and one set of MotionValues. If you want to keep the same logic for setting up both you could do something like saving the generated MotionValues to a WeakMap that uses window/ref.current as the lookup.

Can we utilize useResize hook and implement ResizeObserver inside that?

I'd prefer not to involve ResizeObserver beyond how you've set it up now. I think that strikes a good balance.

@souporserious
Copy link
Contributor Author

Thanks for taking a look! Sounds good! I'll set up the WeakMap today :)

@souporserious
Copy link
Contributor Author

I looked into adding WeakMap support and I'm a little stuck 馃. Since ref comes through as null on the first read there's no key to set for the map which blows other things up that are relying on motion values being created on the first pass. Any ideas here?

This is my simple implementation for the WeakMap portion so far:

const motionValues = new WeakMap()

function getViewportMotionValues(element) {
  if (!motionValues.has(element)) {
    motionValues.set(element, {
      scrollX: motionValue(0),
      scrollY: motionValue(0),
      scrollXProgress: motionValue(0),
      scrollYProgress: motionValue(0),
    })
  }
  return motionValues.get(element)
}

@mattgperry
Copy link
Collaborator

Ah yeah I hadn't considered that. Maybe they are treated slightly differently then, with an event handler created for each element but window only getting a single set/motion values

@mattgperry mattgperry closed this Jul 18, 2019
@mattgperry mattgperry reopened this Jul 18, 2019
@mattgperry
Copy link
Collaborator

Closed this by accident -_-

@nvh nvh added the in progress A fix is in development label Jul 19, 2019
@souporserious
Copy link
Contributor Author

souporserious commented Jul 28, 2019

@InventingWithMonster been a little busy, apologies for the delay! Added support for reusing window scroll values. Not sure how to go about typing the event callback and ResizeObserver doesn't seem to have first-class type support from what I could find. Also, TypeScript is complaining about ref.current possibly being undefined, but the invariant should catch that so not sure what to do there either 馃槄.

@souporserious
Copy link
Contributor Author

馃憢 just checking in on this PR. Any interest in this still?

@joebentaylor1995
Copy link

How can you achieve this without this PR?

@lmartins
Copy link

Would it be possible to get an idea if this is something coming to Framer. PR seems stalled, just looking to understand if worth waiting for it.

@breadadams
Copy link

I could also do with this feature on a project, can I be of any assistance in getting things moving again? 馃槆

@mattgperry mattgperry changed the base branch from master to feature/element-scroll May 15, 2020 09:41
@mattgperry mattgperry merged commit 733fcf0 into framer:feature/element-scroll May 15, 2020
@mattgperry
Copy link
Collaborator

Sorry for the delay on this!

mattgperry added a commit that referenced this pull request May 15, 2020
* feat(use-viewport-scroll): support passing element ref (#195)

* feat(use-viewport-scroll): support passing element ref

* reuse window scroll values

* Adding useScroll hook

* Removing useScroll in lieu of useElementScroll

* Updating changelog

* Fixing generated type import

* Updating API

Co-authored-by: Travis Arnold <ftntravis@gmail.com>
@souporserious souporserious deleted the viewport-scroll-ref-support branch May 15, 2020 15:05
@thebuilder
Copy link
Contributor

thebuilder commented May 28, 2020

This change introduces useLayoutEffect to the the useViewportScroll hook, which causes a React warning when rendering serverside.

Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://fb.me/react-uselayouteffect-ssr for common fixes.

A common fix is to create a useIsomorphicLayoutEffect that selects the right effect for the env.

const useIsomorphicLayoutEffect = isBrowser ? useLayoutEffect : useEffect

function Comp() {
  useIsomorphicLayoutEffect(() => {
    // ...
  })
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress A fix is in development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] use closest scroll container in useViewportScroll
8 participants