Skip to content

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
Contributor

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 { 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
Contributor

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
Contributor

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
Contributor

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 👍

@chrispader chrispader requested a review from lfkwtz September 14, 2023 12:41
@lfkwtz
Copy link
Contributor

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
Contributor

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
Contributor

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

2 participants