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

[HOLD for payment 2024-03-06] [$500] Date of birth - Lower portion of year in the list only appears after some delay #34763

Closed
2 of 6 tasks
kbecciv opened this issue Jan 18, 2024 · 44 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jan 18, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.27-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal team
Slack conversation:

Action Performed:

Precondition: User has not set a birth date.

  1. Go to Settings > Profile.
  2. Go to Personal details.
  3. Go to Date of birth.
  4. Click Year.

Expected Result:

The list of years will appear without delay.

Actual Result:

The lower portion of year in the list only appears after some delay.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6346934_1705606156600.20240118_205138.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013e5606f3817f8af2
  • Upwork Job ID: 1748069670930415616
  • Last Price Increase: 2024-01-25
  • Automatic offers:
    • dukenv0307 | Contributor | 0
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 18, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Copy link

melvin-bot bot commented Jan 18, 2024

Job added to Upwork: https://www.upwork.com/jobs/~013e5606f3817f8af2

@melvin-bot melvin-bot bot changed the title Date of birth - Lower portion of year in the list only appears after some delay [$500] Date of birth - Lower portion of year in the list only appears after some delay Jan 18, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 18, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Jan 18, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Date of birth - Lower portion of year in the list only appears after some delay

What is the root cause of that problem?

The main problem with issue is that we have problem with optimization on this screen
Since the selected year is by default at the end of the list and to show a lot of elements SelectionList needs more time as a result we have empty space

What changes do you think we should make in order to solve the problem?

To fix this issue we can pass shouldUseDynamicMaxToRenderPerBatch for SelectionList

<SelectionList
shouldDelayFocus
textInputLabel={translate('yearPickerPage.selectYear')}
textInputValue={searchText}
textInputMaxLength={4}
onChangeText={(text) => setSearchText(text.replace(CONST.REGEX.NON_NUMERIC, '').trim())}
inputMode={CONST.INPUT_MODE.NUMERIC}
headerMessage={headerMessage}
sections={sections}
onSelectRow={(option) => props.onYearChange(option.value)}
initiallyFocusedOptionKey={props.currentYear.toString()}
showScrollIndicator
shouldStopPropagation
/>

The same param we use for StateSelectorModal or CountrySelectionPage ( We had the same problem in these places )

Also I think we can check other place where we use SelectionList and enable this param
Or enable this param by default

What alternative solutions did you explore? (Optional)

NA

Before

Screen.Recording.2024-01-19.at.08.30.28.mov

After

Screen.Recording.2024-01-19.at.08.31.06.mov

@muas19
Copy link

muas19 commented Jan 18, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Delay to populate the list of years in Date of birth field

What is the root cause of that problem?

When selecting the D.O.B in settings page, we are using a wrong prop variable for minDate and maxDate:
minDate={subYears(new Date(), CONST.DATE_BIRTH.MAX_AGE)} maxDate={subYears(new Date(), CONST.DATE_BIRTH.MIN_AGE)} />
In this file DateOfBirthPage.js

What changes do you think we should make in order to solve the problem?

We should change the prop types to expect constants as follows:
<InputWrapper ... minDate={CONST.minDate} maxDate={CONST.maxDate} />

What alternative solutions did you explore? (Optional)

N/A

Screenshot:

Screen.Recording.2024-01-22.at.10.09.26.PM.mov

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 19, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

The lower portion of year in the list only appears after some delay.

What is the root cause of that problem?

The year 2019 is selected by default on this screen, it's the last year in the list so it takes time for the list to render all elements up to that year, causing the blank issue.

We have an UX issue on this page:

  1. The list is currently from the furthest years (1874) till the most recent years, this is not ideal because most users of our app is less that 70 years old, means most of them will have to scroll at least half the list to get to their potential years, the youngest users have to scroll almost the entire list to get to their years

  2. The default selected option is 2019, which means we're expecting the default user of our app is 5 years old which is not realistic.

What changes do you think we should make in order to solve the problem?

  1. The list should be designed in a way that the most selected items should be on top to minimize user scrolling and unnecessary rendering. This means we need to display the most recent years first, and then the further in the list is further years. We can just reverse the years list to do this (or order from largest to smallest number). We can also add a prop like shouldOrderYearByMostRecent to control this if we want to.

  2. According to this discussion, we want to set it to current year, to do that we just need to set MIN_AGE to 0 here, I've tested and the back-end will accept that date.

  3. We can add shouldUseDynamicMaxToRenderPerBatch to SelectionList in YearPickerModal to mitigate this problem a little, but this will cause more intensive browser CPU usage when first load the list. So I prefer to also fix the UX issue above that leads to this problem.

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@greg-schroeder
Copy link
Contributor

Awaiting proposal review @sobitneupane 👍

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@muas19
Copy link

muas19 commented Jan 22, 2024

Proposal

[Updated] #34763 (comment) With Screenshot

@sobitneupane
Copy link
Contributor

I am working partially this week. I will try to review the proposals before weekend.

Copy link

melvin-bot bot commented Jan 25, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@greg-schroeder
Copy link
Contributor

Let us know if you need help or could use a re-assignment on this one @sobitneupane!

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
@DylanDylann
Copy link
Contributor

I can take over this issue as C+ contributor if @sobitneupane is unavailable

@sobitneupane
Copy link
Contributor

I am back and will review the proposal by EOD tomorrow.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 29, 2024
@greg-schroeder
Copy link
Contributor

Thanks Sobit!

@melvin-bot melvin-bot bot removed the Overdue label Jan 31, 2024
@sobitneupane
Copy link
Contributor

sobitneupane commented Feb 1, 2024

Thanks for the proposal everyone.

Proposal from @ZhenjaHorbach looks good to me. Dynamic maxToRenderPerBatch was implemented by this PR. @ZhenjaHorbach let's look for other component using SectionList which has similar problem.

Copy link

melvin-bot bot commented Feb 1, 2024

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@sobitneupane
Copy link
Contributor

Thank you @tgolen @mountiny for your valuable inputs.

We have discussed above about the @dukenv0307's proposal and all of us believe it is good to make some UX changes in the page. So, we are good to go with @dukenv0307's proposal.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 8, 2024

Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Feb 8, 2024

@tgolen @sobitneupane @greg-schroeder this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 8, 2024
Copy link

melvin-bot bot commented Feb 8, 2024

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added the Overdue label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 12, 2024

@tgolen, @sobitneupane, @greg-schroeder, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@tgolen
Copy link
Contributor

tgolen commented Feb 12, 2024

Daily Update:

  • A contributor was selected and it is waiting for a PR to be drafted. @dukenv0307 do you think that will be coming soon?

@melvin-bot melvin-bot bot removed the Overdue label Feb 12, 2024
@tgolen
Copy link
Contributor

tgolen commented Feb 13, 2024

Daily Update

  • @dukenv0307 Do you have an idea of when you can start on a PR for this?

@dukenv0307
Copy link
Contributor

dukenv0307 commented Feb 13, 2024

Will raise the PR soon after going back from a national holiday on the 15th.

Copy link

melvin-bot bot commented Feb 15, 2024

@tgolen @sobitneupane @greg-schroeder @dukenv0307 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Feb 15, 2024
Copy link

melvin-bot bot commented Feb 15, 2024

Current assignee @sobitneupane is eligible for the Internal assigner, not assigning anyone new.

@dukenv0307
Copy link
Contributor

I'm working on this issue now, will raise PR soon

@dukenv0307 dukenv0307 mentioned this issue Feb 15, 2024
50 tasks
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 15, 2024
@greg-schroeder
Copy link
Contributor

This was merged and deployed to prod 2/28, payment date would be 3/6

@greg-schroeder
Copy link
Contributor

@dukenv0307 - $500 - C
@sobitneupane - $500 - C+

@greg-schroeder greg-schroeder added the Awaiting Payment Auto-added when associated PR is deployed to production label Mar 2, 2024
@greg-schroeder greg-schroeder changed the title [$500] Date of birth - Lower portion of year in the list only appears after some delay [HOLD for payment 2024-03-06] [$500] Date of birth - Lower portion of year in the list only appears after some delay Mar 2, 2024
@greg-schroeder greg-schroeder removed the Reviewing Has a PR in review label Mar 2, 2024
@greg-schroeder
Copy link
Contributor

Paid, and Sobit you can make a manual request!

@JmillsExpensify
Copy link

$500 approved for @sobitneupane based on summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests