Add additional events for controlling picker indicator on web #15673

redstar504 · 2023-03-06T01:00:08Z

Details

I had to work through this a bit more to make the picker indicator behave consistently all platforms. As proposed, I added two new event handlers specific to the web version of the app that update the isOpen state, but they needed to be adapted a bit from what I suggested.

The original proposal worked to solve the reported issue, but it was causing the picker to remain highlighted on desktop web after selecting an item. To solve this, I switched from using onClick to onMouseDown. This solved it simply because of the order the event handlers are called in.

In addition, I included a new onChange event handler whose presence overrides the onValueChange handler for web. This was necessary because my original solution of setting isOpen to false in the onValueChange event was resulting in the indicator changing back to the default setting whenever a new item was scrolled to in the iOS picker wheel.

Fixed Issues

$ #15506
PROPOSAL: #15506 (comment)

Tests

Login to the app and navigate to any page containing a Picker component.
In this issue, we used the Settings > Workspace > Reimburse Expenses picker for selecting mileage units.
Open the picker and confirm that the green indicator beneath the picker is bright green while the picker is open.
Select an item. Confirm that after selecting an item the indicator returns to it's default dark green color.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Login to the staging environment and navigate to any page containing a Picker component.
In this issue, we used the Settings > Workspace > Reimburse Expenses picker for selecting mileage units.
Open the picker and confirm that the green indicator beneath the picker is bright green while the picker is open.
Select an item. Confirm that after selecting an item the indicator returns to it's default dark green color.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Safari

border-mac-safari.mp4

Chrome

Having an issue with OBS not displaying <select> elements during a windowed screen recording (seems like select elements are considered a separate window), but still demonstrates the functionality:

pf-desktop-web.mp4

Mobile Web - Chrome

pf-android-chrome.mp4

Mobile Web - Safari

border-ios-safari.mp4

Desktop

pf-mac-desktop.mp4

iOS

pf-ios-native.mp4

Android

pf-android-native.mp4

MelvinBot · 2023-03-06T01:00:13Z

@marcaaron @thesahindia One of you needs to 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]

MelvinBot · 2023-03-06T01:00:14Z

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

thesahindia · 2023-03-06T18:13:43Z

@redstar504, the picker stays highlighted when we close it without changing the value

Screen.Recording.2023-03-06.at.11.36.13.PM.mov

I think we should handle this case as well.

cc: @marcaaron for thoughts

src/components/Picker.js

marcaaron · 2023-03-06T18:37:11Z

src/components/Picker.js

+                                const index = e.target.selectedIndex;
+                                const value = e.target.options[index].value;
+                                this.onInputChange(value, index);
+                                this.setState({isOpen: false});


It feels like we are sneaking in some platform dependent code here, but it's not clear which is the web or native implementation. Normally, how we deal with that is create a "base" component and then an index.js and index.native.js components. In a file like Picker.js we'd then just import the Picker but the platform dependent changes would be contained in the respective versions of the picker. Would this approach work here?

Yeah that would be better. Also let's create a function in index.js file and call it in onChange (just to make this a little bit cleaner)

marcaaron · 2023-03-06T18:42:58Z

@thesahindia I don't feel super strongly about solving that case in this ticket. We didn't spot it until now and it's not clear what the solution would be - so I say we continue here and then file it as a separate bug. Unless @redstar504 has some idea on how to tackle it and is willing to look into it.

thesahindia · 2023-03-06T18:45:31Z

That makes sense to me!

redstar504 · 2023-03-07T09:37:32Z

@marcaaron @thesahindia Thanks for the input guys. I have been exploring ways to split up the platform-specific code as requested. It appears that our goal is to share the Picker component across all platforms while providing a means to specify any platform-specific event handlers through their respective index files.

Would it make sense to refactor Picker.js into Picker/BasePicker.js, containing all of the platform independent handlers, and then import BasePicker into the platform specific index files? We could pass any platform specific event handlers to BasePicker as props to enable customization for each platform.

thesahindia · 2023-03-07T16:53:41Z

Would it make sense to refactor Picker.js into Picker/BasePicker.js

Yes!

redstar504 · 2023-03-08T05:59:09Z

My latest commit includes platform specific event handling for the Picker component. The implementation involves wrapping the refactored BasePicker with platform specific entrypoint files, and wrapping the entrypoints with a higher order component that supplies default events for both platforms. Web specific events are encapsulated in the index.js file. Native events are supplied as defaults through the HOC since they supplement the web-based events.

Testing on all platforms was successful, and I have included new recordings in my checklist.

@marcaaron @thesahindia

marcaaron · 2023-03-08T18:16:47Z

src/components/withPickerEvents.js

+import {pickerPropTypes, pickerDefaultProps} from './Picker/pickerPropTypes';
+
+export default function (WrappedComponent) {
+    class WithSharedEvents extends Component {


I haven't delved deeply into the reasons for needing it, but is an HOC necessary? Feels like the "WrappedComponent" is always going to be the Picker.

Additionally, I'm having trouble following some of the conventions used here e.g. the callbackConsumer() (stuck out to me as unusual for our codebase and my instincts are telling me it could confuse people).

I'd have to do a deeper dive to come up with more concrete suggestions, but just wanted to toss that observation out there that maybe we could simplify this.

The HOC wraps the Picker in both entrypoints (index.js and index.native.js), so that we can share all of the code contained in it between them. This also gives us a way to manipulate the events passed down to BasePicker using an entrypoint file. I originally had placed the HOC in the Picker directory because it would only ever be used for the Picker, but I noticed all of the others were contained in the base src directory so I moved it there.

An HOC would seem like overkill but I am kind of stuck between a dead simple solution (the one I originally provided), and an over-engineered solution. I have been having trouble finding something in the middle ground that won't introduce a lot of code duplication. Another option might be class inheritance.

We basically need a way to share all of the event handlers between both entrypoints but give the web entry point a way to extend/override them before passing them down to BasePicker. If you have any alternative ideas for that please let me know. If we stick with the HOC I can definitely rename callbackConsumer back to onInputChange.

An HOC would seem like overkill but I am kind of stuck between a dead simple solution (the one I originally provided), and an over-engineered solution. I have been having trouble finding something in the middle ground that won't introduce a lot of code duplication. Another option might be class inheritance.

Yeah I agree the original solution was simple, this feels a little over-engineered now. I am going to defer to @marcaaron on this.

Ok I see the issue now - sorry for the distraction here. I think where this is at is a bit hard to reason about for not much gain.

Here's what I'm thinking as an alternative...

Start by reverting what you have here back to 15fb9c7

Create some prop like shouldUseAdditionalPickerEvents on the Picker

Override the methods that way inside the Picker

It might seem vain, but we can get around the cross platform file extension convention this way and it will allow web/native to switch on/off the functionality we need without specifically referencing the platform anywhere.

@marcaaron Thank you for the advice. I just need a bit more clarification:

It might seem vain, but we can get around the cross platform file extension convention this way and it will allow web/native to switch on/off the functionality we need without specifically referencing the platform anywhere.

Are you suggesting not having platform-specific entry points in this case? I am just curious what is going to toggle shouldUseAdditionalPickerEvents if we don't, and if we didn't, where do we define the web-specific events?

Sorry, I meant something like...

/Picker/index.js -> <Picker shouldUseAdditionalPickerEvents /> /Picker/index.native.js -> <Picker />

@marcaaron Ok I will give that a shot. Since it's just a conditional in the entry points, is this implying that we keep the web events defined in the Picker itself? Just want to clarify exactly what we're after to avoid further delays.

implying that we keep the web events defined in the Picker itself

That was my meaning yea.

But I thought of something that is slightly better and might result in fewer changes while avoiding a reference to the web specific e.target...

Create getAdditionalPickerEvents()

have native return {}

have web return something like

// getAdditionalPickerEvents/index.js function getAdditionalPickerEvents(onMouseDown, onChange) { return { onMouseDown, onChange: (e) = { if (e.target.selectedIndex === undefined) { return; } const index = e.target.selectedIndex; const value = e.target.options[index].value; onChange(value, index); }, }; }

Then pass them like:

pickerProps={{ ...getAdditionalPickerEvents(this.setIsOpen, this.updateAndClose),

This feels slightly better to me as it manages the web event outside of the cross platform Picker code. Sorry for all the back and forth. Our style guide sometimes paints us into a corner 😄

@marcaaron I like the looks of that. I have been trying to find a way to pass down the events from the web entry, but also to be able to pass a function back that can enable/disable the highlight without having to reference them from inside the shared Picker. This looks like it could work.

Also, no worries. I am happy to work towards something we can all be satisfied with.

redstar504 · 2023-03-10T04:31:17Z

That concept worked perfectly @marcaaron, nice job.

The way I implemented it is by passing the additionalPickerEvents containing the web events through props from the index.js entrypoint. In the Picker, I declared additionalPickerEvents as a defaultProp with an empty function, so we don’t have to pass anything from the native entrypoint.

I refactored all of the setState({isOpen: bool}) calls into setters like in your example. I did not introduce an updateAndClose() function, as the native implementation does not handle both operations in a single event, so it would have only been used for web.

Again, checks out on all platforms.

marcaaron

Looks great to me. Just a couple of places where I think we could use a bound function instead of an anonymous arrow function.

src/components/Picker/Picker.js

redstar504 · 2023-03-10T19:32:25Z

@marcaaron Requested changes pushed!

marcaaron

Didn't test but code LGTM so approving. All yours @thesahindia.

thesahindia · 2023-03-12T12:58:22Z

src/components/Picker/Picker.js

+    setIsOpen() {
+        this.setState({
+            isOpen: true,
+        });
+    }
+
+    setIsClosed() {
+        this.setState({
+            isOpen: false,
+        });
+    }


The names here sounds confusing. When I looked at them I felt that we have two different state (isClosed and isOpen) I think we should change the names for these methods or we can just use one method (setIsOpen) and pass true/false as argument.

What do you guys think?

cc: @marcaaron @redstar504

You are right, I think we should clarify what that state variable actually controls. Since it is only responsible for enabling and disabling the highlight, we can make it more understandable by using an isHighlighted state variable, and calling enableHighlight and disableHighlight to control it.

thesahindia · 2023-03-12T13:17:34Z

src/components/Picker/Picker.js

+import Text from '../Text';
+import styles from '../../styles/styles';
+import themeColors from '../../styles/themes/default';
+import {ScrollContext} from '../ScrollViewWithContext';

 const propTypes = {


Let's create a new file pickerPropTypes.js and define all the props there except additionalPickerEvents. Let's import the propTypes and defaultProps from pickerPropTypes.js and use them at native and index.js too. Here's an example.

Since no props are specifically referenced in the platform-specific wrappers, it might be unnecessary to perform validation there before passing them to the BasePicker. However, if this is an established pattern in the codebase, it would be a good idea to follow that with our changes as well.

It looks like we are doing it but I am not sure if it's needed. I think @marcaaron would know more about it or we can ask at expensify-open-source

I think I don't feel too passionate about this one. Or at least I can't think of a situation where it would help because the props are just getting passed straight to the BasePicker. If there was some prop validation warning we would see it either way.

redstar504 · 2023-03-13T01:50:55Z

Thanks for the review @thesahindia. I have added your requested changes to the latest commit.

thesahindia · 2023-03-13T19:56:55Z

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.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-03-14.at.12.41.51.AM.mov

Mobile Web - Chrome

Screen.Recording.2023-03-14.at.1.21.04.AM.mov

Mobile Web - Safari

Screen.Recording.2023-03-14.at.1.23.21.AM.mov

Desktop

Screen.Recording.2023-03-14.at.12.41.04.AM.mov

iOS

Screen.Recording.2023-03-14.at.1.25.37.AM.mov

Android

Screen.Recording.2023-03-14.at.1.10.13.AM.mov

thesahindia · 2023-03-13T20:01:33Z

Looks good! Just one console error -
Steps:-

Reload the page
Go to settings > workspaces > click on any workspace
Click on workspace avatar

redstar504 · 2023-03-14T01:37:48Z

@thesahindia

The error is due to the prop validation we've added in the entrypoints. Adding propTypes directly to the component created by forwardRef does not validate as expected. It seems that we would have to wrap the BasePicker, do the prop validation and then wrap it again using forwardRef. This would be unnecessary, since all of the props are already being validated in the BasePicker. There is no need to validate them in the entrypoints as well.

For example, this works but does not provide any benefit:

// index.js
import {defaultProps, propTypes} from './pickerPropTypes';

// eslint-disable-next-line react/jsx-props-no-spreading
const Picker = props => <BasePicker {...props} additionalPickerEvents={additionalPickerEvents} />;

Picker.propTypes = propTypes;
Picker.defaultProps = defaultProps;

// eslint-disable-next-line react/jsx-props-no-spreading
export default forwardRef((props, ref) => <Picker {...props} innerRef={ref} />);

Using shared propTypes looks to be common when there are multiple entrypoints that do not wrap an underlying component. It's purpose would be to give them a common interface to conform to.

I suggest that we rollback this change as it is not necessary for our scenario.

redstar504 · 2023-03-14T02:33:17Z

@thesahindia I reverted shared prop types due to the reasons described in my last comment. Props are still validated as expected through the base Picker component, and there are no console errors. So we should be good.

thesahindia

I take back what I said. It didn't test well. I was checking other pages to confirm that all the functionalities are working as expected. It is not working correctly. Not getting the error message for the picker. @redstar504, can you look into it?

I am on a slow internet so I couldn't upload the video. You can try reproducing it at "Add debit card page" or any other page where we are validating.

redstar504 · 2023-03-14T15:37:45Z

@thesahindia Great job catching that. We needed to pass the ref all the way down to the base component by that name to continue forwarding it, and only then it's passed to the RNPickerSelect as innerRef. It should be fixed now.

thesahindia

Thanks! It works well.

cc: @marcaaron

marcaaron

Nice job here overall and LGTM. Thanks @thesahindia for the comments and testing 🙇

OSBotify · 2023-03-15T05:26:55Z

✋ 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 · 2023-03-15T12:02:26Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.2.85-0 🚀

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

OSBotify · 2023-03-16T11:11:18Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.2.85-1 🚀

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

redstar504 requested a review from a team as a code owner March 6, 2023 01:00

melvin-bot bot requested review from marcaaron and thesahindia and removed request for a team March 6, 2023 01:00

marcaaron reviewed Mar 6, 2023

View reviewed changes

marcaaron assigned redstar504 Mar 6, 2023

marcaaron reviewed Mar 8, 2023

View reviewed changes

redstar504 added 2 commits March 9, 2023 17:30

Add additional events for controlling picker indicator on web

e00e207

Add web-platform specific Picker events using HOF

6e3b8fc

redstar504 force-pushed the Brayden-PickerIndicator branch from 40c36bc to 6e3b8fc Compare March 10, 2023 04:26

marcaaron requested changes Mar 10, 2023

View reviewed changes

src/components/Picker/Picker.js Show resolved Hide resolved

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

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

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

Use bound functions for setting opened state

b59cfb6

marcaaron self-requested a review March 10, 2023 19:57

marcaaron previously approved these changes Mar 10, 2023

View reviewed changes

thesahindia reviewed Mar 12, 2023

View reviewed changes

Clarify highlighted state, reusable props

49ce444

redstar504 dismissed marcaaron’s stale review via 49ce444 March 13, 2023 01:38

Revert shared propTypes

de32de4

This comment was marked as outdated.

Sign in to view

thesahindia suggested changes Mar 14, 2023

View reviewed changes

redstar504 force-pushed the Brayden-PickerIndicator branch from ebd3673 to 2924efb Compare March 14, 2023 15:42

Pass ref prop properly to base component

f956e52

redstar504 force-pushed the Brayden-PickerIndicator branch from 2924efb to f956e52 Compare March 14, 2023 15:43

thesahindia approved these changes Mar 14, 2023

View reviewed changes

marcaaron approved these changes Mar 15, 2023

View reviewed changes

marcaaron merged commit 38defc6 into Expensify:main Mar 15, 2023

OSBotify mentioned this pull request Mar 15, 2023

Deploy Checklist: New Expensify 2023-03-15 #15983

Closed

8 tasks

MelvinBot mentioned this pull request Mar 16, 2023

[HOLD for payment 2023-03-23] [$1000] Inconsistency while selecting dropdown in mWeb and Android #15506

Closed

6 tasks

thesahindia mentioned this pull request Apr 3, 2023

On web the picker stays highlighted when you close it #16716

Closed

6 tasks

jjcoffee mentioned this pull request Apr 20, 2023

[$1000] Dropdown remains focused if closed without any selection change on android chrome #17438

Closed

6 tasks

Add additional events for controlling picker indicator on web #15673

Add additional events for controlling picker indicator on web #15673

Conversation

redstar504 commented Mar 6, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Safari

Chrome

MelvinBot commented Mar 6, 2023

MelvinBot commented Mar 6, 2023

thesahindia commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Mar 6, 2023

thesahindia commented Mar 6, 2023

redstar504 commented Mar 7, 2023

thesahindia commented Mar 7, 2023

redstar504 commented Mar 8, 2023

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

Choose a reason for hiding this comment

redstar504 Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redstar504 commented Mar 10, 2023

marcaaron left a comment

Choose a reason for hiding this comment

redstar504 commented Mar 10, 2023

marcaaron 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redstar504 commented Mar 13, 2023

thesahindia commented Mar 13, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

thesahindia commented Mar 13, 2023 • edited Loading

redstar504 commented Mar 14, 2023

redstar504 commented Mar 14, 2023 • edited Loading

This comment was marked as outdated.

thesahindia left a comment

Choose a reason for hiding this comment

redstar504 commented Mar 14, 2023

thesahindia left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

OSBotify commented Mar 15, 2023

OSBotify commented Mar 15, 2023

OSBotify commented Mar 16, 2023

redstar504 commented Mar 6, 2023 •

edited

Loading

redstar504 Mar 9, 2023 •

edited

Loading

thesahindia commented Mar 13, 2023 •

edited

Loading

thesahindia commented Mar 13, 2023 •

edited

Loading

redstar504 commented Mar 14, 2023 •

edited

Loading