Feat: Scroll to TextInput onOpen picker #3
Conversation
| import { defaultStyles } from './styles'; | ||
| import { Dimensions } from 'react-native'; | ||
|
|
||
| const IOS_MODAL_HEIGHT = 262; |
There was a problem hiding this comment.
How did you come up with a value of 262?
There was a problem hiding this comment.
I tried to programatically measure the modal instead, but didn't succeed.
Instead i thoroughly tested this value on multiple iOS devices, since the height of the picker modal is the same on all (iOS) devices. Works the same way on iPhone 8:
Simulator.Screen.Recording.-.iPhone.8.-.2022-12-14.at.23.32.16.mp4
There was a problem hiding this comment.
OK, thanks! Can you please add that context as a code comment?
|
We will also need a plan to merge this same change into the upstream library so we don't have to keep maintaining our fork forever. Could you please add that plan to the PR description? |
| @@ -1,7 +1,18 @@ | |||
| module.exports = { | |||
| extends: 'ls-react', | |||
| parserOptions: { | |||
There was a problem hiding this comment.
Is there a specific reason the lint settings are being changed? I think that for the sake of trying to also get this merged into the upstream fork, then fewer changes will result in a better chance of getting it merged.
There was a problem hiding this comment.
The eslint config repo (https://github.com/lawnstarter/eslint-config-ls-react got removed/archived completely, so you can't build the project without changing this.
Maybe the author uses this as a private library, so i can exclude those changes from the upstream PR. Still, the upstream repo has not had any changes since 2 years, so i don't know how successful the upstream PR will be
There was a problem hiding this comment.
OK, got it. Thanks for explaining! It's probably OK then. We could also consider making this change in a separate PR to decrease the scope/review of each PR.
There was a problem hiding this comment.
Actually, now that I think about this a little more... I would like them split out into separate PRs. I think that will help maintaining the changes in this fork easier. Could you please create a separate PR with just the ESlint change, and then I'll merge that, then come back here and merge this one.
There was a problem hiding this comment.
All right, that's merged now if you want to go ahead and merge main back into this PR to clean up the diff.
There was a problem hiding this comment.
Oh, never mind remerging main, I see the diff is fine now.
chrispader
force-pushed
the
fix/scroll-to-input-on-open
branch
from
a3341fd to
48a788b
Compare
|
Looks like you just need to clean up the conflict here and then this is ready to merge. |
|
done! |
2 participants
Needed for Expensify/App#13514
Implements automatic scrolling to
TextInputonce Modal is opened.Also fixes some problems with
eslintin this repo, sinceeslint-config-ls-starterrepo got removed.Upstream Merge Plan
The upstream library is pretty much dead (the last changes were 2 years ago), but still i'd create a feature request issue and a associated PR over there.
Since we don't know, if these changes get merged anytime soon, i think we'll have to keep maintaining this fork too.