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

Hooks > useScrollProgress #82

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

j5gomes
Copy link
Member

@j5gomes j5gomes commented Feb 3, 2025

Changes made in this PR:

  • Adds the useScrollProgress hook;

@j5gomes j5gomes self-assigned this Feb 3, 2025
@j5gomes j5gomes requested review from andrre-ls and removed request for andrre-ls February 3, 2025 16:13
@j5gomes j5gomes marked this pull request as draft February 3, 2025 16:25
@j5gomes j5gomes requested a review from andrre-ls February 3, 2025 16:27
@j5gomes j5gomes marked this pull request as ready for review February 3, 2025 16:27
Copy link
Contributor

@andrre-ls andrre-ls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good push on this! I left some comments with suggestions on how we can take this to the next level.

In case you didn't already know, motion's useScroll hook also allows you to measure horizontal scroll progress. It might be a good reference on how to structure and improve this hook.

Comment on lines +3 to +8
type ScrollProgressCallback = (
scrollLeft: number,
width: number,
scrollWidth: number,
progress: number
) => void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure of these arguments should be rethought. If I want to get to the progress — arguably the most useful argument — I have type out (scrollLeft, width, scrollWidth, progress) => {}, with three of those being unused variables.

Comment on lines +41 to +65
### Basic Usage

```tsx
useScrollProgress(
carouselRef,
(scrollLeft, width, scrollWidth, progress) => {
if (progressLineRef.current) {
progressLineRef.current.style.width = `${progress * 100}%`;
}
}
);
```

### Custom Proportional Scroll Behavior

```tsx
useScrollProgress(
carouselRef,
(scrollLeft, width, scrollWidth, progress) => {
if (progressLineRef.current) {
progressLineRef.current.style.width = `${(width / scrollWidth + scrollLeft / scrollWidth) * 100}%`;
}
}
);
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more useful to have a runnable preview here, instead of just code snippets.


---

## Usage Examples
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Usage Examples
## Examples

More inline with the remaining docs.

- `scrollWidth` (`number`): The total scrollable width of the element.
- `progress` (`number`): A value between `0` and `1`, representing the scroll progress.

This allows you to handle scrolling declaratively with state updates or imperatively using the callback for optimized performance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's a good idea to have scroll progress directly set react state. With how fast scroll (and resize) event listeners trigger, it might cause significant performance issues and therefore be an easy way for a developer to shoot himself in the foot.

If we are to keep the react state, besides documenting potential performance implications, we should throttle the scroll listener and debounce the resize listener.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this only tracks horizontal scroll, I suggest we rename the hook to reflect that (e.g. useHorizontalScrollProgress). However, it would be far more interesting to implement support for tracking progress in both axis, which is something motion's hook does: https://motion.dev/docs/react-use-scroll

definition={{
ref: {
type: "React.RefObject<HTMLDivElement>",
description: "A reference to the element being tracked for scroll progress.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: "A reference to the element being tracked for scroll progress.",
description: "A reference to the element being tracked for scroll progress.",
required: true

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

Successfully merging this pull request may close these issues.

2 participants