migrate AccountType step to FormProvider #56464
Conversation
melvin-bot
bot
requested review from
dominictb
and removed request for
a team
|
@dominictb 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] |
|
@mkzie2 Seems like we have an extra step here: 
Can you think of any way to eliminate it? |
|
@dominictb This is the same step like we did for the type business step in connect US bank account flow or some others that use |
|
I'm not sure what you're asking, can you elaborate? |
Screen.Recording.2025-02-11.at.15.57.27.movDo you agree with the behavior in this PR (Option 2)? |
|
@cead22 Can you confirm the final expectation? |
|
@cead22 Friendly bump |
|
Hmmm, I've seen list pages where you select an option and still have to click the green button to "accept" it. But if you say we're doing what you showed on the video above in other places, specially if those places are part of this flow, then I think that's fine |
|
@joekaufmanexpensify I don't think extra step is allowed here. cc @Expensify/design to confirm this. cc @dominictb |
|
Hmm I feel like we've gone back and forth on this one... I don't know that we ever settled on an official rule for every single flow, cc @Expensify/design for thoughts The original thinking for having a green confirm button at the bottom of these flows was that if you were inside of a stepper flow (with multiple steps...) it might feel nicer to always show a green button at the bottom so you had consistency between pages. But I think we've also kinda walked back on that and we've found examples of where it feels a little cumbersome (like the onboarding modal) where we would prefer not to need the green confirm button on list selections. However, if you access a list selection via a push row, we never use a green confirmation button. That's something we've been consistent with for a long time I think. I'm not sure if that is helpful or not, but that's the backstory AFAIK. |
|
I agree with everything Shawn said, but I'm not sure that gets us closer to a decision haha.
This pattern does kinda seem to apply here though? |
|
Yeah it's basically here that we're at a crossroads: I think as long as you have a way to go back and change your decision, I would be fine removing the green button at the bottom. And I believe at the end of this flow you would see a confirmation screen that allows you to change the decision, so maybe we'd be good there. We don't have that kind of confirmation for the onboarding modal though... but at the same time, I think we've had discussions about wanting to remove the extra click? Curious what the other designers think though! |
|
Yeah good point. I think the ability to change your decision after the fact makes it fine to remove this green button. |
|
Hmm, based on this, it seems like we might be trying to add an additional push row for the account type. So you'd land on the page to select account type and then have another push row that brings you to where you actually make a selection (if I am understanding correctly). That seems redundant to me, and I agree with @shubham1206agra that we should have you select the account type from the top-level page, rather than forcing you down another level of navigation to select it. How it works now is below. We added the But not a fan of adding an extra push row, like was being proposed here. 2025-02-14_10-20-54.mp4 |
|
Ah great context Joe. I'm on board with everything you said. Definitely agree about removing the unnecessary nested push row, and in a flow like this where most screens need a bottom button, it makes sense to me to just keep the whole form/flow using that pattern. And then if we do go that route, on the confirm screen, if you tap back into it, I'd be fine with keeping that screen the same as during the flow (and not removing the bottom button). Does that all make sense? Alternatively, we could keep the green button during the initial flow, but if you revisit from the confirm screen we could remove the bottom button and treat it like all our other push-row-to-select-list screens, just not sure that's entirely necessary. |
|
Thank for all informations, I will give an update soon. |
|
Cool, yeah I can totally get on board with all of that too Joe & Danny. So it sounds like... just keep doing things the way we've been doing them 🤣 |
|
@dominictb Updated to remove the extra step. |
|
Just chiming in to say that I think having the button at the bottom for consistency is the way to go here as well 👍 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-02-18.at.16.45.29.movAndroid: mWeb ChromeScreen.Recording.2025-02-18.at.16.39.36.moviOS: NativeScreen.Recording.2025-02-18.at.16.42.20.moviOS: mWeb SafariScreen.Recording.2025-02-18.at.16.33.26.movMacOS: Chrome / SafariScreen.Recording.2025-02-18.at.16.22.25.movMacOS: DesktopScreen.Recording.2025-02-18.at.16.48.44.mov |
|
@mkzie2 Please update the screenshots with the latest change. |
@dominictb I updated. |
|
I have a few clarifying questions because these changes look different from what I expected after reading the proposal
|
Updated the test step and the video on the web for it. |
|
Thanks for the reply. I have some follow ups because I'm still confused, so I'll try asking a different way
There's no other way to do this? We have to
As opposed to clicking on the menu item, and then having to click a button to confirm, right?
What is "menu item" and what is "modal" here?
Why can't we use |
| ); | ||
|
|
||
| return ( | ||
| <SelectionList |
There was a problem hiding this comment.
@cead22 Thanks for your feedback, let me clarify it again.
There's no other way to do this? We have to ValuePicker as the InputComponent to make this work with FormProvider behave and work like TypeBusiness?
ValuePicker is an existing component; we can reuse this in the AccountType step when migrating to FormProvider.
As opposed to clicking on the menu item, and then having to click a button to confirm, right?
What is "menu item" and what is "modal" here?
Yes, here is the result of my first implementation
Screen.Recording.2025-02-11.at.15.57.27.mov
- And this is the
menu item
- This is the
modal
The main idea of the last implementation is creating a way to only show the selection list without the extra step of opening the modal. Then I implement it in ValuePicker with a new prop so we can use this in the feature if we want. And the result will be like this
Why can't we use SelectionList in ValuePicker?
As the idea above, we only need to reuse the SelectionList part on ValueSelectorModal and render it in ValuePicker if shouldShowModal props is false
You can see here ValueSelectionList used the SelectionList. The purpose of this component is prevent the duplicate code when I add both SelectionList in ValuePicker and ValueSelectorModal.
There was a problem hiding this comment.
@cead22 Any thoughts on my comment above.
There was a problem hiding this comment.
Sorry for the delay, it's been a busy few days.
There's no other way to do this? We have to ValuePicker as the InputComponent to make this work with FormProvider behave and work like TypeBusiness?
ValuePickeris an existing component; we can reuse this in theAccountTypestep when migrating toFormProvider.
To clarify, are there alternative ways to implement this? If so, can you share why they're not as good as this one? If there aren't, can you explain why?
Sorry if this is all known to others, but I'm not super well versed on all the components that exists and how they work together
The main idea of the last implementation is creating a way to only show the selection list without the extra step of opening the modal.
Just to make sure I understand, the selection list looks very similar to the modal, and the main difference is that the modal has a green button you need to click after making the selection?
Then I implement it in
ValuePickerwith a new prop so we can use this in the feature if we want. And the result will be like this
Again just so I learn, if we didn't want to do this in anticipation of using it in the future, how would the code be different? Not saying we should change it, I just want to understand exactly what's needed for now, vs what we're doing to set this up to be re-used
Why can't we use SelectionList in ValuePicker?
As the idea above, we only need to reuse theSelectionListpart onValueSelectorModaland render it inValuePickerifshouldShowModalprops isfalse
I didn't quite follow this. Can we use SelectionList in ValuePicker? Does that question make sense?
The purpose of this component is prevent the duplicate code when I add both
SelectionListinValuePickerandValueSelectorModal.
👍 thanks, I get this now
There was a problem hiding this comment.
To clarify, are there alternative ways to implement this? If so, can you share why they're not as good as this one? If there aren't, can you explain why?
Sorry if this is all known to others, but I'm not super well versed on all the components that exists and how they work together
We can create a new component and use it as InputWrapper of the FormProvider in the AccountType step. But we're good to use the ValuePicker because we can see the SelectionList inside AccountType (the code on the latest main) is the same with the selection list inside ValueSelectorModal of ValuePicker
The main idea of the last implementation is creating a way to only show the selection list without the extra step of opening the modal.
Just to make sure I understand, the selection list looks very similar to the modal, and the main difference is that the modal has a green button you need to click after making the selection?
The main idea is to keep the current UI and behavior in the AccountType step, but using FormProvier with ValuePicker instead of the current code here. And then when using the ValuePicker, we need a way only to display the SelectionList, then the behavior is the same as we have on the main, making a selection and clicking on the confirm button to go to the next step.
Again just so I learn, if we didn't want to do this in anticipation of using it in the future, how would the code be different? Not saying we should change it, I just want to understand exactly what's needed for now, vs what we're doing to set this up to be re-used
In the feature, if we have a substep with a single field and we need to make a selection, we can reuse the ValuePicker with shouldShowModal as false to remove the extra step.
- what's needed for now: Now we need an
InputWrappercomponent that is only a selection list like the current selection in here. And becauseValuePickeralready had the same list like this, we can use this component with introducing a new prop to simply return the selection list if this prop isfalse
- what we're doing to set this up to be re-used: I set this up to be re-used by introducing the
shouldShowModal. If we have a substep that has the same UI and behavior like thisAccountTypestep, we can re-use like this.
There was a problem hiding this comment.
To clarify, are there alternative ways to implement this? If so, can you share why they're not as good as this one? If there aren't, can you explain why?
We can create a new component and use it asInputWrapperof theFormProviderin theAccountTypestep. But we're good to use theValuePickerbecause we can see theSelectionListinsideAccountType(the code on the latest main) is the same with the selection list insideValueSelectorModalofValuePicker
Sorry, I still don't understand this. When I asked about whether there's another way, I meant with existing logic/components. I don't think this addresses why the current solution is better (apologies if it does)
The main idea is to keep the current UI and behavior in the
AccountTypestep, but usingFormProvierwithValuePickerinstead of the current code here. And then when using theValuePicker, we need a way only to display theSelectionList, then the behavior is the same as we have on the main, making a selection and clicking on the confirm button to go to the next step.
Sorry about this one too, but I'm not sure if/how this answers my question. What does "the extra step of opening the modal" mean in "The main idea of the last implementation is creating a way to only show the selection list without the extra step of opening the modal."?
- what we're doing to set this up to be re-used: I set this up to be re-used by introducing the
shouldShowModal. If we have a substep that has the same UI and behavior like thisAccountTypestep, we can re-use like this.
Thanks for this explanation. Is this something we anticipate using right away, or should we add it when we need it?
I'm gonna find a better reviewer for this PR. I don't want to keep delaying it, but I also don't want to merge code I don't understand
|
@mkzie2 what I got from the discussions is that we're trying to go for this kind of behavior. If so, can we just follow the same pattern we have in WorkspaceNewRoomPage.tsx Screen.Recording.2025-03-05.at.9.31.34.AM.mov |
That is my initial implementation 410488570-6755f844-af00-4ca8-a715-19b359592ae7.mov@luacmartins The expected we want here still keeps the current behavior (remove the extra step) but will use the Screen.Recording.2025-02-18.at.17.23.22.mov |
|
I think it should be like below, and once you tap on savings and current we go back to the other page, without having to tap the confirm button. That aligns with @shawnborton 's comment here
|
|
I agree with @cead22. It'd be great to get @Expensify/design's confirmation on that too |
@cead22 @luacmartins We confirmed here that still keep the confirm button |
Explanation of Change
migrate AccountType step to FormProvider
Fixed Issues
$ #55688
PROPOSAL: #55688 (comment)
Tests
Precondition: Enable
newDotInternationalDepositBankAccountbetaOffline tests
Same
QA Steps
Precondition: Enable
newDotInternationalDepositBankAccountbetaPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2025-02-18.at.17.31.02.mov
Android: mWeb Chrome
Screen.Recording.2025-02-18.at.17.26.00.mov
iOS: Native
Screen.Recording.2025-02-18.at.17.32.45.mov
iOS: mWeb Safari
Screen.Recording.2025-02-18.at.17.28.35.mov
MacOS: Chrome / Safari
Screen.Recording.2025-02-18.at.17.23.22.mov
Screen.Recording.2025-02-20.at.23.23.19.mov
Screen.Recording.2025-02-20.at.23.25.01.mov
MacOS: Desktop
Screen.Recording.2025-02-18.at.17.35.46.mov