-
Notifications
You must be signed in to change notification settings - Fork 501
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
Fix: Scroll to TextInput on picker opening #496
base: master
Are you sure you want to change the base?
Fix: Scroll to TextInput on picker opening #496
Conversation
this is cool - can you fix the conflicts? |
@lfkwtz conflicts resolved 👍 |
src/index.js
Outdated
import PropTypes from 'prop-types'; | ||
import isEqual from 'lodash.isequal'; | ||
import { Picker } from '@react-native-picker/picker'; | ||
import { defaultStyles } from './styles'; | ||
|
||
// Measuring the modal before rendering is not working reliably, so we need to hardcode the height | ||
// This height was tested thoroughly on several iPhone Models (from iPhone 8 to 14 Pro) |
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.
tested with iPhone SE / Mini variants too?
also - does landscape mode work?
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.
also - ipads?
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.
yes, it was compatible, but i just made some improvements 👍 🚀
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.
great. able to add tests?
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.
hey, sorry for the late reply...
i think it's difficult to add tests here, because this is basically an iOS specific feature. With jest
/enzyme
we can only test in a mocked environment.
I added a dummy test, that reflects a test for this functionality, but this will not work in a web environnment
@lfkwtz just found a reliable way to measure the actual modal height, without the need to hardcode it. Let me know what you think and if everything works for you 👍 |
looking good - able to close that coverage gap? |
Not sure how, if were not able to test this lib natively on iOS... Do you have an idea?� |
@lfkwtz do you think we can merge this? Absolutely no priority though.. |
i currently do not have access to merge until all checks are passed. so we'll have to work through that first. |
I'm gonna try to continue working on the tests if i get the chance any time soon... |
looks like the team at lawnstarter is starting to work on this a bit more so maybe they will be able to help you get this in cc @alberto-mourao-lawnstarter |
ah awesome! Yea, if they could help me with fixing/setting up these tests, that would be great! thanks |
gonna resolve merge conflicts soon |
Problem
When the picker gets opened, the
<ScrollView />
which contains the<RNPickerSelect />
isn't responding to the modal opening.Since using the picker should feel like using keyboard, we need to have some logic to directly scroll to the location of the
<TextInput />
in the containing<ScrollView />
.Solution
I added the code needed, to allow this kind of functionality. There are two props needed:
scrollViewRef
: A ref to the containing<ScrollView />
component, so we can manuall call.scrollTo()
scrollViewContentOffsetY
: the current scrolling offset, which can be retrieved by using theonScroll
callback on the<ScrollView />
(Lmk if you have a solution, that requires less props or is easier)
This is completely optional, so if these 2 props are not provided, the behaviour of this picker library won't change
Notices
Hope this PR brings some useful functionality.