Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for @0xmiroslav payment] Make Profile > Personal Details > Address true push-to-page fields. #17548

Closed JmillsExpensify closed 1 year ago

JmillsExpensify commented 1 year ago

@Beamanator @cristipaval I was in a thread with @shawnborton and he noticed that both our State and Country fields are not true push-to-page inputs. Mind helping us understand why that's the case? Ideally if we're going to allow selection of a list, we'd follow a true push-to-page pattern where the row/input has a right caret, upon which tapping that takes you to the full list on a separate page. Right? Adding a New Feature label on this one given that it's not technically a bug.

Screenshot 2023-04-17 at 17 50 04

In addition to the above, let's make sure we have 20px below each input grouping (could be just an input, or input + hint text), as @shawnborton asked here.

MelvinBot commented 1 year ago

Current assignee @JmillsExpensify is eligible for the NewFeature assigner, not assigning anyone new.

JmillsExpensify commented 1 year ago

Went ahead and assigned myself so no one is assigned and we align on next steps.

cristipaval commented 1 year ago

I think we refactored the parent Profile page but we missed this page. It looks like there was an issue created for this but there was confusion here and the issue was closed. cc @Beamanator

JmillsExpensify commented 1 year ago

Ok no worries. We can just use this issue to close the loop. Sound good?

cristipaval commented 1 year ago

Sounds good to me. I can take this one and offload it to Callstack, as it is just front-end work.

Prince-Mendiratta commented 1 year ago

Hi @JmillsExpensify @cristipaval! I'd like to work on this, would it be possible to make this issue external or will you be going ahead with Callstack?

gedu commented 1 year ago

Hey hey, Edu here, I am from Callstack - expert contributor group, I can take a look at this one

cristipaval commented 1 year ago

Hey @gedu! Thanks for taking this over! Push-to-page design means that the user should be redirected to a new page to select the option, instead of the system picker. Take a look at how pronouns and Timezone selectors work in the Profile page.

gedu commented 1 year ago

@cristipaval I'm not seeing that State has a dropdown where is that or how can I reproduce it? (I'm up to date with main)

Screen Shot 2023-04-20 at 15 55 25
cristipaval commented 1 year ago

You first have to select the country, which I think isn't intuitive and also is not user-friendly to go down to the latest field, select an option, and then go back up to the State field to select an option. @JmillsExpensify @shawnborton What do you think? Shouldn't we reorder the fields to have Country before State, since the State options depend on the Country selection?

shawnborton commented 1 year ago

If we do that, I think I would opt to just have country be the first input of the entire form.

JmillsExpensify commented 1 year ago

Are we using this same page/form for business bank accounts, or only for personal details? If so, I think that might be confusing – that is, unless we default to the United States (we only support USD bank accounts).

gedu commented 1 year ago

For bank account is using Plaid, not sure if you can connect using another way. If you wanna add a debit card, it uses another component and structure, so moving the Country up will be only for personal details.

JmillsExpensify commented 1 year ago

For bank account is using Plaid, not sure if you can connect using another way.

Ah yes we start with Plaid but immediately after that verification move to an Address page. As long as this is only for personal details, I think it makes sense.

gedu commented 1 year ago

chatting internally for a proposal

gedu commented 1 year ago

Proposal

Please restate the problem we are trying to solve with this issue. Currently, both the State and Country fields are not true push-to-page inputs. To follow a true push-to-page pattern, we need to display an item that tapping on it should take the user to the full list on a separate page.

What is the root cause of this problem? The root cause of the problem is that we are using the Picker components for the State and Country fields.

What changes do you think we should make to solve this problem? To solve this problem, we can make the Country and State fields look like menu items (MenuItemWithTopDescription).

We should display the State field as a menu item only when the selected country is the USA. Additionally, I would move the Country field up, before the State field. If the selected country is the USA, the State field should be another push-to-page component showing the USA states. We should use two different UI components for the State UI - a menu item when the USA is selected, and a normal input for other countries.

When the user clicks on the Country Menu Item, they will navigate to another page using ROUTES.SETTINGS_COUNTRY. In the new page, we will use the OptionsSelector component and fill it with props.translate(‘allCountries’). We will create another page for the USA states, ROUTES.SETTINGS_USA_STATES, and use the same OptionsSelector component filled with props.translate(‘allStates’). We can also modify the StatePicker component to apply the push-to-page pattern on the Add a Debit Card page, which also uses the same component.

To send back the selected value, we will use the navigate with concatenation method, like so:

AddressPage Routes.navigate(${ROUTES.SETTINGS_COUNTRY}?goBackTo=${Navigation.getActiveRoute()}) CountryPage Routes.navigate(${routes.params.goBackTo}?country=${selectedCountry})

We will handle this the same way it is handled by the CalendarPicker with YearPickerPage.

MelvinBot commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

cristipaval commented 1 year ago

I like your proposal @gedu ! Go ahead with it 🚀

cristipaval commented 1 year ago

FYI: I changed the description of the issue to include what @shawnborton asked here.

gedu commented 1 year ago

@cristipaval awesome I will check it too.

Quick question, in my proposal I wanna use MenuItemWithTopDescription but what do you think?

https://user-images.githubusercontent.com/1676818/234910975-c8cee3b2-2beb-4ec7-a7bb-88a3bac42711.mp4

or should I try to use another component?

gedu commented 1 year ago

@cristipaval Hey hey, I think I like this way, what do you think?

https://user-images.githubusercontent.com/1676818/235114942-9b310d86-e680-4573-af1e-33914cb7729d.mp4

cristipaval commented 1 year ago

Hey @gedu! Use MenuItemWithTopDescription. You could use negative margins to get everything well aligned.

gedu commented 1 year ago

Hey @gedu! Use MenuItemWithTopDescription. You could use negative margins to get everything well aligned.

Awesome, thanks

gedu commented 1 year ago

Still working on it, doing some final changes and updates.

I hope to get a PR soon

gedu commented 1 year ago

@cristipaval PR is ready to reaview

gedu commented 1 year ago

@cristipaval @JmillsExpensify The PR is ready

cristipaval commented 1 year ago

Hey @gedu, thanks for the PR! @mananjadhav is the C+ assigned to firstly review it. The thing is that we are focusing these days on some other urgent tasks and there might be some delay. Sorry for that.

mananjadhav commented 1 year ago

I'llhave it reviewed and tested by tomorrow.

mananjadhav commented 1 year ago

@cristipaval @JmillsExpensify can we please reassign another C+ here? I haven't been getting time to look at this and I might be OOO for two days.

cristipaval commented 1 year ago

Thanks for the heads-up @mananjadhav

melvin-bot[bot] commented 1 year ago

This issue has not been updated in over 15 days. @JmillsExpensify, @gedu, @cristipaval eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] commented 1 year ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

mountiny commented 1 year ago

Not true

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.46-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-08-03. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @gedu, @mountiny, @0xmiroslav Huh... This is 4 days overdue. Who can take care of this?

JmillsExpensify commented 1 year ago

Alright, let's circle back and get payments issued, right? @0xmiroslav Do you want to similarly have me put this issue on hold for your payment, like other ones?

0xmiros commented 1 year ago

yes still on hold. I did initial review. @s77rt continued review to the end

JmillsExpensify commented 1 year ago

Ah interesting. Not sure how we handle those. I'll ask internally and circle back.

mountiny commented 1 year ago

I think we can split up the payment to make it fair

JmillsExpensify commented 1 year ago

As in, $500 and $500?

mountiny commented 1 year ago

yes

JmillsExpensify commented 1 year ago

Ok cool. That works for me. Adding a payment summary for this issue:

Upwork job is here: https://www.upwork.com/jobs/~01b61b644b462b028a and I already sent an offer to @s77rt. @0xmiroslav Will you be requesting payment via NewDot?

s77rt commented 1 year ago

Should the payment here be $250 - $250? We have this regression.

s77rt commented 1 year ago

Oh actually we have also another one https://github.com/Expensify/App/pull/21049#discussion_r1295771691

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @gedu, @mountiny, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @gedu, @mountiny, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

mountiny commented 1 year ago

$250 - $250 sounds good

mountiny commented 1 year ago

@JmillsExpensify bump on this one

melvin-bot[bot] commented 1 year ago

@JmillsExpensify, @gedu, @mountiny, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...