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

Modernize library #11

Merged
merged 13 commits into from
Jul 18, 2020
Merged

Modernize library #11

merged 13 commits into from
Jul 18, 2020

Conversation

shawnmcknight
Copy link
Owner

This PR completely updates the library to support React >= 16.8.0. Essentially everything has been rewritten from the ground up. The repo has been converted from JavaScript to TypeScript, all of the tooling has been refreshed and updated, and the component is now a functional component using React hooks.

** Breaking Changes **
The API for the component has changed. It no longer accepts both onChange and onLoad props, instead only using a single onChange prop which serves both purposes. Additionally, the onChange callback no longer accepts an object with property names scrollbarHeight and scrollbarWidth, instead using the shorter height and width.

@kgregory
Copy link
Collaborator

Nice job

@Noitidart
Copy link

This is awesome, do you have plans for a hook for this?

@kgregory
Copy link
Collaborator

@Noitidart I was toying with that idea. I may submit a PR

@Noitidart
Copy link

Thank you for your awesome work and help to the community! I want to PR this thing to use your library because I dont think it handles adjust on zoom. https://github.com/theKashey/react-remove-scroll-bar

@kgregory
Copy link
Collaborator

kgregory commented Jan 12, 2021

I've whipped up a quick test to see if I could create such a hook: https://codesandbox.io/s/react-scrollbar-size-hook-mw6pf

Take a look.

Since our method of measuring the scrollbar in the component involves adding an element to the DOM, this hook would have to do the same. Not really a problem if it cleans up after itself.

The benefit of the hook is that it requires way less from the consumer. Since it manages its own state, consumers can simply invoke the hook to access the scrollbar height and width.

Any objections to adding a hook to supplement the component, @shawnmcknight?

Oh and I realized that we should probably be adding aria-hidden to the elements that we render and hide (for accessibility reasons). It's probably best that these elements are taken out of the accessibility tree since they're not meant to be seen by the user and exist for taking measurements.

@shawnmcknight
Copy link
Owner Author

Any objections to adding a hook to supplement the component, @shawnmcknight?

No objections at all. I guess we would export both the component and the hook for use? I think as long as we make sure to reuse as much code as possible in both places to maintain consistency this would be a very good update.

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