-
Notifications
You must be signed in to change notification settings - Fork 931
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
feat(scrollIntoView): add new prop to override built-in scrollIntoView #538
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This seems like a nice trade-off. Since this is compiled out of the React Native build it shouldn't affect that as well.
Unfortunately, since the type is function(node: HTMLElement, rootNode: HTMLElement)
, it won't ever be usable on React Native. On React Native, this could use ReactNative.findNodeHandle(component)
to get the position of the element, or FlatList.scrollToItem()
/FlatList.scrollToIndex()
if we got the item or index out of this method.
I'll table that for another issue or PR though!
Good thinking @eliperkins! It'd be cool to get official support for scrolling to the highlighted index with ReactNative! |
Codecov Report
@@ Coverage Diff @@
## master #538 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 349 349
Branches 84 84
=====================================
Hits 349 349
Continue to review full report at Codecov.
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
We probably also needs to change the type definition for this. Will do this in a bit. |
downshift-js#538) **What**: feat(scrollIntoView): add new prop to override built-in scrollIntoView **Why**: Closes downshift-js#537 **How**: React makes this easy. I leverage `defaultProps` to set our own implementation of `scrollIntoView` as the `scrollIntoView` prop and then call it via `this.props.scrollIntoView` so users of downshift can override it to be whatever they want. This means that I had to change the test slightly because the `scrollIntoView` function is no longer called directly on `utils` so we can't spy on it. Just swapped that to use mocking and it's working fine. **Checklist**: - [x] Documentation - [x] Tests - [x] Ready to be merged - [ ] Added myself to contributors table N/A
What: feat(scrollIntoView): add new prop to override built-in scrollIntoView
Why: Closes #537
How: React makes this easy. I leverage
defaultProps
to set our own implementation ofscrollIntoView
as thescrollIntoView
prop and then call it viathis.props.scrollIntoView
so users of downshift can override it to be whatever they want.This means that I had to change the test slightly because the
scrollIntoView
function is no longer called directly onutils
so we can't spy on it. Just swapped that to use mocking and it's working fine.Checklist: