Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tracker] Selection List Refactor #11795

Closed
mountiny opened this issue Oct 13, 2022 · 155 comments
Closed

[Tracker] Selection List Refactor #11795

mountiny opened this issue Oct 13, 2022 · 155 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@mountiny
Copy link
Contributor

mountiny commented Oct 13, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

melvin-bot bot commented Oct 13, 2022

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.

@JmillsExpensify JmillsExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2022

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member Monthly KSv2 labels Oct 18, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@rushatgabhane
Copy link
Member

rushatgabhane commented Oct 20, 2022

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

@JmillsExpensify
Copy link

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
Copy link
Contributor Author

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
Copy link

@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:

  • All editable fields within a form are shown as an input with a right caret
  • Tapping a field pushes to a new page to edit that field or related fields (i.e. Personal address splits out to Address/Zip/State/Country)
  • The first field within the new page is in focus by default, and making a selection or saving an entry returns the user to the previous form automatically.

@JmillsExpensify
Copy link

JmillsExpensify commented Oct 20, 2022

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
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added the Overdue label Oct 20, 2022
@iwiznia
Copy link
Contributor

iwiznia commented Oct 21, 2022

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
Copy link
Contributor

Following this discussion before I can assign an engineer.

@melvin-bot melvin-bot bot removed the Overdue label Oct 21, 2022
@rushatgabhane
Copy link
Member

much appreciated! @JmillsExpensify

@twisterdotcom
Copy link
Contributor

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
Copy link
Contributor

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?

@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2022
@Beamanator
Copy link
Contributor

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

@isabelastisser isabelastisser removed the Daily KSv2 label Oct 24, 2022
Copy link

melvin-bot bot commented Dec 12, 2023

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Dec 14, 2023

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Dec 18, 2023

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@lukemorawski
Copy link
Contributor

chilax @MelvinBot, the issue is being discussed in a different thread

Copy link

melvin-bot bot commented Dec 25, 2023

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Dec 27, 2023

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski Eep! 4 days overdue now. Issues have feelings too...

Copy link

melvin-bot bot commented Dec 29, 2023

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Copy link

melvin-bot bot commented Jan 2, 2024

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski 10 days overdue. I'm getting more depressed than Marvin.

Copy link

melvin-bot bot commented Jan 4, 2024

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 8, 2024
Copy link

melvin-bot bot commented Jan 8, 2024

This issue has not been updated in over 14 days. @dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski eroding to Weekly issue.

Copy link

melvin-bot bot commented Feb 1, 2024

This issue has not been updated in over 15 days. @dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski 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!

@roryabraham
Copy link
Contributor

BaseOptionsSelector just got migrated from class => functional component. There's a number of active bugs / regressions with it, and we're feeling the need for some cleanup: https://expensify.slack.com/archives/C01GTK53T8Q/p1708995608771049

@melvin-bot melvin-bot bot closed this as completed Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

@dannymcclain, @twisterdotcom, @cristipaval, @ArekChr, @Santhosh-Sellavel, @lukemorawski, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@roryabraham
Copy link
Contributor

I think we should work to get this over the finish line. BaseOptionsSelector is a very large, complex component to leave in place deprecated.

@twisterdotcom
Copy link
Contributor

Who is working on this now?

@roryabraham
Copy link
Contributor

I'm not sure when it happened, but BaseOptionsSelector has been removed, so I think this is done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests