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

feat(scrollIntoView): add new prop to override built-in scrollIntoView #538

Merged
merged 1 commit into from Aug 10, 2018

Conversation

kentcdodds
Copy link
Member

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 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:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table N/A

Copy link
Contributor

@eliperkins eliperkins left a 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!

@kentcdodds
Copy link
Member Author

Good thinking @eliperkins! It'd be cool to get official support for scrolling to the highlighted index with ReactNative!

@kentcdodds kentcdodds merged commit afc4853 into master Aug 10, 2018
@kentcdodds kentcdodds deleted the pr/add-scroll-into-view-prop branch August 10, 2018 19:08
@codecov
Copy link

codecov bot commented Aug 10, 2018

Codecov Report

Merging #538 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #538   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         349    349           
  Branches       84     84           
=====================================
  Hits          349    349
Impacted Files Coverage Δ
src/downshift.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65b2b0c...d5df367. Read the comment docs.

@kentcdodds
Copy link
Member Author

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franklixuefei
Copy link
Collaborator

We probably also needs to change the type definition for this. Will do this in a bit.

Rendez pushed a commit to Rendez/downshift that referenced this pull request Sep 30, 2018
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
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.

None yet

3 participants