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.54k stars 2.88k forks source link

[Tracker] Selection List Refactor #11795

Closed mountiny closed 1 month ago

mountiny commented 2 years ago

Problem

OptionRow component is a beast that handles all the use cases of the App where there is a list of options. This is error-prone and hard to maintain.

Solution

Let's refactor all different list component variations into 3 new, clean components:


Other Related issues

Potentially updating the List button background styles as per this Slack thread.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c464f37c9f85723e
  • Upwork Job ID: 1620157290858266624
  • Last Price Increase: 2023-01-30
shawnborton commented 2 years ago

Ah interesting, I was thinking the main issue is mostly because we have a list selection on the screen at the same time as a separate text area. So I wonder if a solution to this whole thing would be to rethink the invite flow where maybe the personal message and selection rows are not on the same screen?

mountiny commented 2 years ago

So I wonder if a solution to this whole thing would be to rethink the invite flow where maybe the personal message and selection rows are not on the same screen?

that is also part of the problem I would say. Can we combine other inputs with the list input on the same screen? How will it behave? I think this will benefit from the discussion about the push to page navigation decisions

melvin-bot[bot] commented 2 years 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.

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @isabelastisser (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

rushatgabhane commented 2 years ago

@mountiny who is leading this issue / how i can follow the predesign?

JmillsExpensify commented 2 years ago

Ah interesting, I was thinking the main issue is mostly because we have a list selection on the screen at the same time as a separate text area. So I wonder if a solution to this whole thing would be to rethink the invite flow where maybe the personal message and selection rows are not on the same screen?

Oh haha yeah totally agree. I just got here from the most recently added issue and came to the same conclusion. Wouldn't the custom message be a selection row and push-to-page?

mountiny commented 2 years ago

Wouldn't the custom message be a selection row and push-to-page?

Makes sense, this should be discovered in the pre-design. @rushatgabhane I think there is nobody to lead this yet, @isabelastisser might need to try and get some volunteer from engineers as I dont have a time at the moment to take this on :/

JmillsExpensify commented 2 years ago

@rushatgabhane I don't think we've done a good job of pre-designing this in #expensify-open-source, but to give you an example of a change we're thinking of making.

Existing Implementation

Screen Shot 2022-10-20 at 14 03 38

Planned future implementation image (5)

The tl;dr on "push to page" for forms is this:

JmillsExpensify commented 2 years ago

In terms of next steps, I was just chatting with @vitHoracek in person and I think we really need to decide whether this initiative is part of improving quality/performance/polish, or if it's not.

Building on that, we've done pretty extensive designs internally for how to make lists/forms/dates/etc. work more consistently than they do today. It's just we paused all of that for the current WhatsApp Quality initiative. As a result, this entire initiative around rationalizing forms has been on hold.

One alternative we could potentially pursue is this: Update the parts of Account Settings (plus any other areas this touches) for consistency only. So we wouldn't add OldDot features like secondary logins, or even personal details. But we could still "polish" navigation and editing in this part of the app by bringing it in line with push-to-page.

Thoughts? CCing people that have participated in this discussion for visibility before we move the convo to #expensify-open-source. @kevinksullivan @zanyrenney @Beamanator @trjExpensify @iwiznia @twisterdotcom

trjExpensify commented 2 years ago

It makes sense to me, though at this point, we may as well just implement account settings minus Personal details. We have a concept of adding a secondary login (i.e Phone number) today, with a button to Resend the validation link etc, so we would need to move that somewhere if we update to push-to-page.

iwiznia commented 2 years ago

we've done pretty extensive designs internally for how to make lists/forms/dates/etc. work more consistently than they do today

IMO this is part of the polish of WAQ and this issue too.

Update the parts of Account Settings (plus any other areas this touches) for consistency only. So we wouldn't add OldDot features like secondary logins, or even personal details. But we could still "polish" navigation and editing in this part of the app by bringing it in line with push-to-page.

Agree

isabelastisser commented 2 years ago

Following this discussion before I can assign an engineer.

rushatgabhane commented 2 years ago

much appreciated! @JmillsExpensify

twisterdotcom commented 2 years ago

What would this amount to then?

  1. Move First name and Last name into their own Display name drawer
  2. Move Pronouns into its own Pronouns drawer
  3. Move Timezone into its own Timezone drawer
  4. Move Email and Phone into it's own Contact drawer

For the latter, would we follow the existing Contact Methods design, or would we keep the settings in the drawer like this?

image
trjExpensify commented 2 years ago

For the latter, would we follow the existing Contact Methods design, or would we keep the settings in the drawer like this?

I kinda' think we should follow the Contact Methods design, otherwise the Resend button for the phone number secondary login will be inconsistent with the rest of the updates.

Also, do we need to add a 5. for making sure we update the Preferences settings page as well?

Beamanator commented 2 years ago

Just catching up here, I also like the idea of moving forward with some parts of Account Settings (basically everything except the "new" Personal Details pages) so we have a solid push-to-page foundation for other pages to mimic 👍

Also, do we need to add a 5. for making sure we update the Preferences settings page as well?

Good question @trjExpensify , would we update the dropdowns to be new pages (via push to page)? The two dropdowns I'm thinking of are:

  1. Priority mode
  2. Language
twisterdotcom commented 2 years ago

would we update the dropdowns to be new pages (via push to page)? The two dropdowns I'm thinking of are:

I think we should, yes, but I agree, we didn't go with this in the N7 doc. We should redesign that screen to use Push to page lists.

This is tantamount to doing everything except Personal Details really from the N7 Account Settings doc. We'll need to handle all the RBR, offline and "Add new contact method" flows to do this properly I think, otherwise, we'll only allow adding one new contact method and then remove the button?

Beamanator commented 2 years ago

I think we should, yes, but I agree, we didn't go with this in the N7 doc. We should redesign that screen to use Push to page lists.

Agreed, it would be great to have all settings pages be aligned on push-to-page implementation, then go from there. We don't have to add the Preferences page changes to the N7 Account Settings doc though, right? 🙏

This is tantamount to doing everything except Personal Details really from the N7 Account Settings doc. We'll need to handle all the RBR, offline and "Add new contact method" flows to do this properly I think, otherwise, we'll only allow adding one new contact method and then remove the button?

Agreed, we're already doing RBR in the App so let's add it to the Contact Methods pages too. I guess if we want to do minimal stuff we coulddd just have the "Contact methods" page & "Contact method details" page (for the resend verification button) without the remove / set as default, right?

twisterdotcom commented 2 years ago

True, we could add Remove and Set as default later.

twisterdotcom commented 2 years ago

Asking about some of the oddities involved here: https://expensify.slack.com/archives/C01GTK53T8Q/p1666881619925549

isabelastisser commented 2 years ago

Not overdue.

kavimuru commented 1 year ago

12668

JmillsExpensify commented 1 year ago

Regarding that issue immediately above, I think we have lots of issues around optionList, the starting point of which is this issue. At some point, we might want to create a tracking for that specific initiative.

isabelastisser commented 1 year ago

Not overdue.

JmillsExpensify commented 1 year ago

Not really related to anything, but I think it would be ideal if we had either BZ members @twisterdotcom or @zanyrenney assigned to this issue, since they are working on the related list improvements that are coming with Account Settings – though of course if @isabelastisser would like to stay assigned that's fine too!

twisterdotcom commented 1 year ago

I won't steal all the credit from Isa, but I'll self-assign too.

Beamanator commented 1 year ago

Noting here that the new Pronouns page just got merged! That leaves just the Timezone page(s) which @cristipaval is working on updating 👍

JmillsExpensify commented 1 year ago

Woo, sweet!

cristipaval commented 1 year ago

Update from my end: Timezone pages are ready, the thing is that I found a bug on Web-Expensify in the UpdateAutomaticTimezone command and I opened a PR with the fix. We need this WE PR to be deployed in order to get Timezone pages ready for review.

isabelastisser commented 1 year ago

Not overdue.

isabelastisser commented 1 year ago

Unassigning myself per Jason's comment here. @twisterdotcom, I haven't participated in the discussion, so take all the credit you can here! :)

JmillsExpensify commented 1 year ago

My two sense. This isn't really a bug as much as a UX improvements. Accordingly, I'm removing the bug label.

JmillsExpensify commented 1 year ago

Also, I added myself so that I can keep this on my radar.

JmillsExpensify commented 1 year ago

Updated the title because I don't think this is a priority right now, and the monthly label also confirms that.

cristipaval commented 1 year ago

Hey @JmillsExpensify, this is already in review. I took it over from @Beamanator and pushed it forward as a WAQ thing, according to what was discussed above 1-2 months ago. Here is the PR.

mountiny commented 1 year ago

I think this issue has a little bit diverted, the issue body is not talking about the same problems your PRs address. I think Jason meant that the OP task is on hold (re-designing the option lists)

mountiny commented 1 year ago

Added https://github.com/Expensify/App/issues/12998 to the list of issues on Hold for this one.

JmillsExpensify commented 1 year ago

This would be a good one to pick up again once WAQ is over and the Account Settings revamp is complete.

JmillsExpensify commented 1 year ago

@twisterdotcom Are you passionate about this one? I'm super short on time, but I do think that this is increasingly a good project to take on.

twisterdotcom commented 1 year ago

I sort of think we can close this can't we? Given we've finished this as part of Account Settings:

image

And we have an issue for Preferences already: https://github.com/Expensify/App/issues/14432

Is there a remaining task to document this standardisation somewhere?

mountiny commented 1 year ago

@twisterdotcom I think this issue was originally more about tracking changes to lists like this

image

Basically such list selection is used in couple places and there is not clear guideline yet about how keyboard navigation should behave for these (going up and down and selecting using Enter).

JmillsExpensify commented 1 year ago

I kind of think we should close this issue. We're in the process of creating testing guidelines for the app.

cristipaval commented 1 year ago

We have an issue which is held on this one. If we close, please help me decide what solution we want for this particular issue.

JmillsExpensify commented 1 year ago

For that one, should we just not scroll to the top on each selection?

cristipaval commented 1 year ago

@shawnborton Instead of creating another follow up issue for aligning the design for selection and option lists in the App, I think we could use this one, now that we're discussing about closing it. I think it's related, if we are fine with how they "work", we would just change the title to Align how list selection "looks". What do you think?

shawnborton commented 1 year ago

That sounds great to me! I think this is generally where we landed with how the lists should look: image

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01c464f37c9f85723e

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @Santhosh-Sellavel (Internal)

cristipaval commented 1 year ago

cc @Beamanator As you mentioned in the PR related to Preferences screen, in the follow-up PR that I'm going to open to fix the current issue I should take care of the following, in addition to what it was discussed together with @shawnborton:

JmillsExpensify commented 1 year ago

I think this issue is still on hold, right?