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

Fix: Scroll to TextInput on picker opening #496

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

chrispader
Copy link

@chrispader chrispader commented Jan 7, 2023

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 the onScroll 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.

@lfkwtz
Copy link
Collaborator

lfkwtz commented Aug 28, 2023

this is cool - can you fix the conflicts?

@chrispader
Copy link
Author

@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)
Copy link
Collaborator

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also - ipads?

Copy link
Author

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 👍 🚀

Copy link
Collaborator

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?

Copy link
Author

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

@chrispader
Copy link
Author

@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 👍

@lfkwtz
Copy link
Collaborator

lfkwtz commented Oct 26, 2023

looking good - able to close that coverage gap?

@chrispader
Copy link
Author

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?�

@chrispader
Copy link
Author

@lfkwtz do you think we can merge this? Absolutely no priority though..

@lfkwtz
Copy link
Collaborator

lfkwtz commented Jan 10, 2024

@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.

@chrispader
Copy link
Author

I'm gonna try to continue working on the tests if i get the chance any time soon...

@lfkwtz
Copy link
Collaborator

lfkwtz commented Mar 29, 2024

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

@chrispader
Copy link
Author

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

@chrispader
Copy link
Author

gonna resolve merge conflicts soon

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

2 participants