Fix: Scroll to Picker TextInput on open #13514

chrispader · 2022-12-11T19:15:17Z

Held on changes to our fork: Feat: Scroll to TextInput onOpen picker react-native-picker-select#3

Details

Implement a custom HOC that allows to automatically scroll ScrollView to react-native-picker-select's TextInput.

Adds a <ScrollViewWithPickers /> component which should be used instead of vanilla ScrollView. This adds a ref and passes it down to the Picker component via ScrollViewWithPickersContext.

<Picker /> component is adjusted, so that scrollViewRef is passed to RNPickerSelect, if it's provided.

Relevant changes in react-native-picker-select library are implemented here

Fixed Issues

$ #11391
Proposal

Tests

Click on "Unit" input in Workflows "Reimburse expenses screen" a few times, with different ScrollView contentOffsets

Go to a the workspace settings
Go to the "Reimburse expenses" setting
Under "Track Distance", tap on the input for "Unit"
For iOS, the entire unit field should be visible above the picker. For all other platforms, there should be no changes to the input or the picker (they will all have different pickers).

Offline tests

None

QA Steps

Same as tests above

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Not needed, as this is (native) iOS specific

Mobile Web - Chrome

Not needed, as this is (native) iOS specific

Mobile Web - Safari

Not needed, as this is (native) iOS specific

Desktop

Not needed, as this is (native) iOS specific

iOS

iPhone 14 Pro:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2022-12-20.at.20.16.29.mp4

iPhone 8:

Simulator.Screen.Recording.-.iPhone.8.-.2022-12-20.at.20.26.06.mp4

Android

Not needed, as this is (native) iOS specific

melvin-bot · 2022-12-11T19:15:22Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

melvin-bot · 2022-12-11T19:15:43Z

@PauloGasparSv Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

PauloGasparSv · 2022-12-12T15:49:23Z

Hey @chrispader :D

Could you update the "Fixed Issue" section to be a dollar sign followed by the issue URL, like in this other P.R.?

I think the C+ member would have been added here if the issue was linked in the $ issueurl format, so let me add @parasharrajat as a reviewer here.

cc @JmillsExpensify @tgolen

tgolen

Looks good! I have just a few minor requests.

I've placed a HOLD on this until the PR is merged in our fork.

src/components/Picker/index.js

src/components/ScrollViewWithPickers.js

src/pages/workspace/WorkspacePageWithSections.js

src/components/ScrollViewWithPickers.js

src/components/Picker/index.js

src/pages/workspace/WorkspacePageWithSections.js

tgolen

Looks pretty good! Thanks for adding it in that other spot.

src/components/ScrollViewWithPickers.js

tgolen · 2022-12-19T17:15:30Z

OK, I have removed the HOLD on this issue, so it looks like the steps here are to:

Point this to the latest version of the expensify fork for the picker (@chrispader )
Finish up the checklist in the PR description and get screenshots added (@chrispader )
Test/review the code (@tgolen @parasharrajat )

chrispader · 2022-12-22T15:01:45Z

@tgolen Added screenshots for iOS (iPhone 14 Pro and iPhone 8) and checked the remaining points in the checklist. I hope the last checkbox

I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

is no trick question.

parasharrajat · 2022-12-22T16:22:46Z

Thanks, for the changes. Please check all the checkboxes in the checklist.

I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

No tricks here. It just asks you to check off all items on the list.

There is nothing to review here. It is just a lib upgrade PR. @tgolen Do you want me to run the tests for the related issue?

JmillsExpensify · 2022-12-22T19:27:06Z

Looks like we still need to circle back and address requested changes plus check all the checkboxes?

src/components/ScrollViewWithPickers.js

Co-authored-by: Tim Golen <tgolen@gmail.com>

chrispader · 2022-12-28T14:04:29Z

@parasharrajat this PR should be ready now!

tgolen · 2022-12-28T16:54:30Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

parasharrajat · 2022-12-28T17:08:30Z

It looks like @tgolen you are going to do the final review and merge based on the comment above. Do you need something from me on this?

tgolen · 2022-12-28T17:11:00Z

Ah yes, I just finished up testing and final review, so I don't need anything else from you @parasharrajat 👍

tgolen · 2022-12-28T17:12:37Z

All PR reviewer checklist items have been completed. Maybe there is a version mismatch on the checklist? Not sure. It's OK to merge with that one failing test though.

OSBotify · 2022-12-28T17:18:17Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-12-28T20:02:49Z

🚀 Deployed to staging by @tgolen in version: 1.2.45-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-12-29T19:41:14Z

🚀 Deployed to production by @chiragsalian in version: 1.2.45-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

aldo-expensify · 2023-01-10T21:46:19Z

src/components/Form.js

@@ -249,7 +250,7 @@ class Form extends React.Component {
    render() {
        return (
            <>
-                <ScrollView
+                <ScrollViewWithContext


This change caused this issue: #13909

Seems to be related to the ref

chrispader requested a review from a team as a code owner December 11, 2022 19:15

melvin-bot bot requested review from PauloGasparSv and removed request for a team December 11, 2022 19:15

chrispader mentioned this pull request Dec 11, 2022

Feat: Scroll to TextInput onOpen picker Expensify/react-native-picker-select#3

Merged

tgolen self-requested a review December 12, 2022 15:47

PauloGasparSv requested a review from parasharrajat December 12, 2022 15:49

PauloGasparSv assigned chrispader Dec 12, 2022

tgolen changed the title ~~Fix: Scroll to Picker TextInput on open~~ [HOLD] Fix: Scroll to Picker TextInput on open Dec 12, 2022

tgolen requested changes Dec 12, 2022

View reviewed changes

parasharrajat suggested changes Dec 12, 2022

View reviewed changes

tgolen mentioned this pull request Dec 13, 2022

[HOLD for payment 2023-01-11] IOS - Workspace -Units under Reimbursement-hidden by keyboard after tap on it (small phones) #11391

Closed

chrispader force-pushed the @chrispader/fix-scroll-to-input-on-picker-open branch from 99aba25 to cc6d9a0 Compare December 14, 2022 21:42

chrispader requested review from tgolen and removed request for PauloGasparSv December 14, 2022 22:35

chrispader force-pushed the @chrispader/fix-scroll-to-input-on-picker-open branch from 9b4f7a1 to a00bd13 Compare December 14, 2022 23:03

tgolen requested changes Dec 15, 2022

View reviewed changes

src/components/ScrollViewWithPickers.js Outdated Show resolved Hide resolved

src/components/ScrollViewWithPickers.js Outdated Show resolved Hide resolved

tgolen changed the title ~~[HOLD] Fix: Scroll to Picker TextInput on open~~ Fix: Scroll to Picker TextInput on open Dec 19, 2022

parasharrajat reviewed Dec 26, 2022

View reviewed changes

src/components/ScrollViewWithPickers.js Outdated Show resolved Hide resolved

parasharrajat reviewed Dec 26, 2022

View reviewed changes

src/components/ScrollViewWithPickers.js Outdated Show resolved Hide resolved

parasharrajat reviewed Dec 26, 2022

View reviewed changes

src/components/ScrollViewWithPickers.js Outdated Show resolved Hide resolved

parasharrajat reviewed Dec 26, 2022

View reviewed changes

src/components/ScrollViewWithPickers.js Outdated Show resolved Hide resolved

Christoph Pader added 2 commits December 28, 2022 14:39

fix: scroll to TextInput on Picker open

a4cae37

fix: simplify null check on scrollViewRef

1bb5e42

Christoph Pader and others added 13 commits December 28, 2022 14:39

fix: formatting

ed9a6cd

fix: formatting

6ba04a2

fix: scrollView ref passing

480139d

pass scrollViewContentOffsetY to Picker

014cb6d

update react-native-picker-select github commit in package-lock.json

2687bda

fix: add scrolling fix for other screens

c5f1d86

Update src/components/ScrollViewWithPickers.js

d60d72a

Co-authored-by: Tim Golen <tgolen@gmail.com>

rename scrollHandler

1e65793

chore: update react-native-picker-select dependency

a89b139

chore: add commit sha

a9d4bf0

fix: wrong repo name

88047d0

chore: update package-lock.json

9ac0ac8

fix: wrong this binding

de1c7e1

chrispader force-pushed the @chrispader/fix-scroll-to-input-on-picker-open branch from 544ce05 to de1c7e1 Compare December 28, 2022 13:39

fix: requested changes

23740e0

fix: rename ScrollViewWithContext component everywhere

858b7a7

tgolen approved these changes Dec 28, 2022

View reviewed changes

tgolen merged commit bccb53b into Expensify:main Dec 28, 2022

OSBotify mentioned this pull request Dec 28, 2022

Deploy Checklist: New Expensify 2022-12-28 #13868

Closed

6 tasks

ali-thowfeek mentioned this pull request Jan 2, 2023

[$1000] Dev - Pressing on ‘fix the errors’ on validation forms throw error message #13909

Closed

6 tasks

aldo-expensify reviewed Jan 10, 2023

View reviewed changes

aldo-expensify mentioned this pull request Jan 19, 2023

Fix: Scroll to picker regression #14409

Merged

54 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Scroll to Picker TextInput on open #13514

Fix: Scroll to Picker TextInput on open #13514

chrispader commented Dec 11, 2022 •

edited by tgolen

Loading

melvin-bot bot commented Dec 11, 2022

melvin-bot bot commented Dec 11, 2022

PauloGasparSv commented Dec 12, 2022

tgolen left a comment

tgolen left a comment

tgolen commented Dec 19, 2022

chrispader commented Dec 22, 2022

parasharrajat commented Dec 22, 2022

JmillsExpensify commented Dec 22, 2022

chrispader commented Dec 28, 2022 •

edited

Loading

tgolen commented Dec 28, 2022 •

edited

Loading

parasharrajat commented Dec 28, 2022 •

edited

Loading

tgolen commented Dec 28, 2022

tgolen commented Dec 28, 2022

OSBotify commented Dec 28, 2022

OSBotify commented Dec 28, 2022

OSBotify commented Dec 29, 2022

aldo-expensify Jan 10, 2023

Fix: Scroll to Picker TextInput on open #13514

Fix: Scroll to Picker TextInput on open #13514

Conversation

chrispader commented Dec 11, 2022 • edited by tgolen Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 11, 2022

melvin-bot bot commented Dec 11, 2022

PauloGasparSv commented Dec 12, 2022

tgolen left a comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

tgolen commented Dec 19, 2022

chrispader commented Dec 22, 2022

parasharrajat commented Dec 22, 2022

JmillsExpensify commented Dec 22, 2022

chrispader commented Dec 28, 2022 • edited Loading

tgolen commented Dec 28, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

parasharrajat commented Dec 28, 2022 • edited Loading

tgolen commented Dec 28, 2022

tgolen commented Dec 28, 2022

OSBotify commented Dec 28, 2022

OSBotify commented Dec 28, 2022

OSBotify commented Dec 29, 2022

aldo-expensify Jan 10, 2023

Choose a reason for hiding this comment

chrispader commented Dec 11, 2022 •

edited by tgolen

Loading

chrispader commented Dec 28, 2022 •

edited

Loading

tgolen commented Dec 28, 2022 •

edited

Loading

parasharrajat commented Dec 28, 2022 •

edited

Loading