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

Add ability to use via custom hook #263

Merged
merged 14 commits into from
Mar 11, 2021
Merged

Add ability to use via custom hook #263

merged 14 commits into from
Mar 11, 2021

Conversation

shawnmcknight
Copy link
Owner

Description 📝

This PR adds a custom hook useScrollbarSize to the exports from the package. The existing component syntax was retained for backwards compatibility, but will be deprecated in a future major release (likely 4.0).

Conceptually, the hook works identically to the component in that it will measure the scrollbar sizes and react to changes which might affect the size of the scrollbar (e.g. change to browser zoom factor). This is intended merely as a usability enhancement as use of the hook is significantly easier than use of the component.

All examples and guides have been updated to favor using the hook instead of the component

Type of change 💎

  • New feature (non-breaking change which adds functionality)

Example code 📜

See changes to README.md for updated examples.

How Has This Been Tested? 🚦

  • Unit tests were created and updated as needed.
  • Both examples (hook and component) were run to ensure everything works as intended.
  • Consumed the library from another project to ensure that it runs properly

Checklist 🏁

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • New and existing unit tests pass locally with my changes
  • My changes pass lint, prettier, and TS checks
  • I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas

@shawnmcknight
Copy link
Owner Author

@kgregory I would appreciate it if you could give this one a review.

@shawnmcknight
Copy link
Owner Author

@Noitidart tagging you since you had asked about this feature.

@Noitidart
Copy link

Thank you. @shawnmcknight - checking now!

Copy link

@Noitidart Noitidart left a comment

Choose a reason for hiding this comment

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

Left some nits :) Thanks for this amazing work!

example/README.md Outdated Show resolved Hide resolved
src/useScrollbarSize.ts Outdated Show resolved Hide resolved
src/useScrollbarSize.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@kgregory kgregory left a comment

Choose a reason for hiding this comment

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

Looks good!

I like @Noitidart's suggestion for local state. I also think it may be best to heed the React warning about referencing a ref's value in the useEffect cleanup.

test/mockDimensions.ts Show resolved Hide resolved
src/useScrollbarSize.ts Show resolved Hide resolved
src/useScrollbarSize.ts Outdated Show resolved Hide resolved
@shawnmcknight shawnmcknight requested a review from kgregory March 11, 2021 01:32
Copy link
Collaborator

@kgregory kgregory left a comment

Choose a reason for hiding this comment

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

Looks good, found a typo that I missed in the initial review, but I'm approving.

example-component/README.md Outdated Show resolved Hide resolved
@shawnmcknight
Copy link
Owner Author

Thanks for the review @kgregory and @Noitidart !

@shawnmcknight shawnmcknight merged commit d0d0ff7 into main Mar 11, 2021
@shawnmcknight shawnmcknight deleted the create-hook branch March 11, 2021 23:12
@shawnmcknight shawnmcknight mentioned this pull request Apr 3, 2021
7 tasks
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