Refactor picker to be a cross-platform component #2242

tgolen · 2021-04-06T17:09:03Z

@nickmurray47 @NikkiWines This PR was based on some stuff that I came across in #1767, primarily some missing method docs. As I looked more at the PR, I spotted a few things that should be improved to match our existing patterns better. Please have a look at this and let me know if you have any questions

Details

Adds some missing method docs
Renames a prop to have a less redundant name
DRY up some logic in the checkbox
Convert the picker into a cross-platform component

Fixed Issues

No issue, just some things I spotted in #1767

Tests / QA Steps

Go to the profile page
Go to preferences
Change the priority mode
Go back to settings
Go to profile
Change preferred prounouns
Change timezones
Change automatic timezone setting
Verify all changes are saved properly

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

nickmurray47 · 2021-04-06T17:48:30Z

src/components/Checkbox.js

+        <Pressable onPress={() => onClick(!isChecked)}>
+            {label ? (


are we hiding the checkbox from now on if there's a label present? I think we should show the Checkmark icon regardless, otherwise, it's hard to know that it's a clickable element

Ah, good catch. I think I read the original code wrong. I need to revert this.

nickmurray47 · 2021-04-06T17:49:16Z

src/pages/home/report/ReportActionPropTypes.js

-    actionName: PropTypes.string.isRequired,
+    actionName: PropTypes.string,


why the switch to making these prop optional?

There was a prop warning in the console that undefined was being passed. I looked through the code and there is a spot where the report action passed is {} (an empty object) so I just corrected the propTypes here.

tgolen

Updated!

tgolen · 2021-04-06T18:06:06Z

src/components/Checkbox.js

+        <Pressable onPress={() => onClick(!isChecked)}>
+            {label ? (


Ah, good catch. I think I read the original code wrong. I need to revert this.

tgolen · 2021-04-06T18:07:27Z

src/pages/home/report/ReportActionPropTypes.js

-    actionName: PropTypes.string.isRequired,
+    actionName: PropTypes.string,


There was a prop warning in the console that undefined was being passed. I looked through the code and there is a spot where the report action passed is {} (an empty object) so I just corrected the propTypes here.

nickmurray47

LGTM and test well on Web and iOS

nickmurray47 · 2021-04-06T19:25:40Z

looks like there are some merge conflicts too @tgolen

tgolen · 2021-04-06T21:00:41Z

OK, updated to fix the conflicts. I added a file rename so that we are using the proper conventions and pointed out some more missing method docs and comments in #2169

Maftalion · 2021-04-07T00:18:25Z

src/components/Picker/index.android.js

+            inputAndroid: [
+                styles.picker.inputAndroid, styles.textInput, styles.disabledTextInput,
+            ],


not sure how platform specific styles are handled but would it make more sense to have the ios/android/index files just export the style and the component itself be defined once? would avoid duplication and make it easier to update without having to touch 3 files

NikkiWines

Two small comments, looking good otherwise.

NikkiWines · 2021-04-07T00:16:48Z

src/components/Picker/index.android.js

@@ -0,0 +1,35 @@
+import React from 'react';


I'm not sure I understand why we need three different, largely identical files for the picker.

is the style the only change? could we have that an input for another compnent perharphs? No real idea, just wandering

Yeah, the styles are basically the only difference. I like the idea of just exporting the proper styles from the platform-specific files and DRYing this up more. I'll be working on that today.

NikkiWines · 2021-04-07T00:17:37Z

src/components/Picker/index.android.js

+        items={items}
+        style={useDisabledStyles ? {
+            ...styles.picker,
+            inputAndroid: [


This can be

inputAndroid: [styles.picker.inputAndroid, styles.textInput, styles.disabledTextInput],

tgolen · 2021-04-08T17:26:15Z

OK, updated and retested on all platforms. I refactored it so the component is only defined once and only the styles are platform specific.

NikkiWines

This looks good to me, any reason that mWeb is marked as n/a?

tgolen · 2021-04-09T20:30:01Z

Just because... laziness 😁 I added testing for mobile web.

sketchydroide

LGTM

isagoico · 2021-04-12T18:50:36Z

We are blocked to check this QA this PR in both iOS and Android apps because of the scrolling issue in settings>profile #2147

isagoico · 2021-04-12T22:47:20Z

QA in other platforms was a pass (mweb, web, desktop)

tgolen added 5 commits April 6, 2021 10:00

Add method docs

3ede3ce

Rename checkbox callback prop

54a0047

Finish renaming the callback prop

037dc18

Fix proptype warnings

b88ba8b

Refactor picker into its own component

1c86bf7

tgolen requested review from NikkiWines and nickmurray47 April 6, 2021 17:09

tgolen requested a review from a team as a code owner April 6, 2021 17:09

DRY render logic

e4bdc92

MelvinBot requested review from sketchydroide and removed request for a team April 6, 2021 17:09

tgolen added 2 commits April 6, 2021 11:09

Remove unused component

d571a87

Add more method docs

ff00697

nickmurray47 reviewed Apr 6, 2021

View reviewed changes

Revert the DRY changes for checkbox

0a1823c

tgolen commented Apr 6, 2021

View reviewed changes

nickmurray47 previously approved these changes Apr 6, 2021

View reviewed changes

Merge branch 'master' into tgolen-improve-profile-quality

cd9618e

tgolen dismissed nickmurray47’s stale review via cd9618e April 6, 2021 20:55

Rename files to match conventions

5a62d87

tgolen self-assigned this Apr 6, 2021

Maftalion reviewed Apr 7, 2021

View reviewed changes

NikkiWines reviewed Apr 7, 2021

View reviewed changes

tgolen added 3 commits April 8, 2021 10:58

Merge branch 'master' into tgolen-improve-profile-quality

dcd92bc

Only have platform specific styles

6abfbb9

Revert changes to lock file

be475cd

Merge branch 'master' into tgolen-improve-profile-quality

d10d340

NikkiWines approved these changes Apr 9, 2021

View reviewed changes

nickmurray47 approved these changes Apr 9, 2021

View reviewed changes

sketchydroide approved these changes Apr 12, 2021

View reviewed changes

tgolen merged commit 377e1c2 into master Apr 12, 2021

tgolen deleted the tgolen-improve-profile-quality branch April 12, 2021 16:35

OSBotify mentioned this pull request Apr 12, 2021

Deploy Checklist: Expensify.cash 2021-04-09 #2332

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor picker to be a cross-platform component #2242

Refactor picker to be a cross-platform component #2242

tgolen commented Apr 6, 2021 •

edited

Loading

nickmurray47 Apr 6, 2021

tgolen Apr 6, 2021

nickmurray47 Apr 6, 2021

tgolen Apr 6, 2021

tgolen left a comment

tgolen Apr 6, 2021

tgolen Apr 6, 2021

nickmurray47 left a comment

nickmurray47 commented Apr 6, 2021

tgolen commented Apr 6, 2021

Maftalion Apr 7, 2021

NikkiWines left a comment

NikkiWines Apr 7, 2021

sketchydroide Apr 7, 2021

tgolen Apr 8, 2021

NikkiWines Apr 7, 2021

tgolen commented Apr 8, 2021

NikkiWines left a comment

tgolen commented Apr 9, 2021

sketchydroide left a comment

isagoico commented Apr 12, 2021

isagoico commented Apr 12, 2021

		actionName: PropTypes.string.isRequired,
		actionName: PropTypes.string,

Refactor picker to be a cross-platform component #2242

Refactor picker to be a cross-platform component #2242

Conversation

tgolen commented Apr 6, 2021 • edited Loading

Details

Fixed Issues

Tests / QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickmurray47 left a comment

Choose a reason for hiding this comment

nickmurray47 commented Apr 6, 2021

tgolen commented Apr 6, 2021

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Apr 8, 2021

NikkiWines left a comment

Choose a reason for hiding this comment

tgolen commented Apr 9, 2021

sketchydroide left a comment

Choose a reason for hiding this comment

isagoico commented Apr 12, 2021

isagoico commented Apr 12, 2021

tgolen commented Apr 6, 2021 •

edited

Loading