[HOLD for payment 2024-01-24] [$500] Settings - Only top 10 timezones have tooltip

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: v1.4.13-0
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:

1. Open the app
2. Open settings->profile->timezone->timezone
3. Hover on timezone values from top, observe that only top 10 values display tooltip

Expected Result:

App should either display tooltip on all timezone values or not display tooltip on any values

Actual Result:

App only displays tooltip on top 10 timezone values

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

Bug6313262_1702569416836.windows_chrome_-_only_top_10_timezone_has_tooltip.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011eddc6038aace445
• Upwork Job ID: 1735696150787383296
• Last Price Increase: 2023-12-15
• Automatic offers:
• jjcoffee | Reviewer | 28066145
• dukenv0307 | Contributor | 28066146

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011eddc6038aace445

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[GItGudRatio]

Looks intentional

App/src/components/SelectionList/BaseSelectionList.js

Line 248 in 8fc594b

// We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade.

[GItGudRatio]

Proposal

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

Settings - Only top 10 timezones have tooltip

What is the root cause of that problem?

This is done intentionally to prevent performance issues where there are hundreds of reports.

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

We can introduce a prop like limitTooltips with default value true and when set to false that will disable this check and use this exclusively for the timezone page.

What alternative solutions did you explore? (Optional)

[dukenv0307]

Proposal

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

App only displays tooltip on top 10 timezone values

What is the root cause of that problem?

Earlier we have this limit to show tooltips only on first 10 user items, this is because we can have hundreds of report items and it can cause performance issue.

But recently we enable tooltips for text selection list as well (like timezone page) via this PR, the text selection list always has a limited number of items, but unfortunately the existing 10 items tooltip limit applies to the newly added tooltips for text selection list.

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

Only applies the 10 items tooltip limit for user list items, for text list items, it should always show tooltip since there's no performance issue and no reason to limit it.

We can do this by replacing this with

const isUserItem = lodashGet(item, 'icons.length', 0) > 0;
const showTooltip = !(isUserItem && normalizedIndex >= 10);

(The logic to check isUserItem is the same as here, we can pass it down as a prop to BaseListItem so we don't have to calculate it again inside BaseListItem)

What alternative solutions did you explore? (Optional)

We can consider disable tooltips for timezone page entirely via a prop, because the tooltip doesn't provide any additional content, the tooltip is supposed to show the full text if the option item has ellipsis text, but in this case the timezone text is always short and will never have ellipsis, so the tooltip doesn't provide any value. There're other pages like this too and we can consider disabling tooltips for them to reduce distraction for users.

[ishpaul777]

i recall there was a discussion about the expected behaviour for a similar case some other place in app, i just dont remember the conclusion we get to... now i can't find the thread.

cc @vitHoracek @trjExpensify since i recall you both were involved

[jjcoffee]

Seems pretty clear from #26091 that the limit of displaying only 10 tooltips was only intended for the workspace members & invite pages. So @dukenv0307's proposal makes the most sense to me as it provides a general solution without needing to set a prop everywhere we use selection lists (as would be the case for this proposal).

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[trjExpensify]

@ishpaul777, I think you meant to tag @mountiny not fake Vit! 😉

I think the discussion you're remembering was around the user lists where we decided that instead of just limiting it to 10 which looks inconsistent in the results, we'd show a tooltip only when the row is being truncated.

[mountiny]

@trjExpensify Yeah

I am wonderin gif this bug fits the wave/ vip criteria.

additionally, the tooltips here are actually quite useless arent they. They only show what user already can see so given they are not the best performance wise, I would even vote to remove them on this list

[dukenv0307]

additionally, the tooltips here are actually quite useless arent they. They only show what user already can see so given they are not the best performance wise, I would even vote to remove them on this list

@mountiny yeah it's indeed quite useless, I suggested the removal of it in my alternate solution as well, we can go with that.

[trjExpensify]

Yeah, I'd much rather remove them. Taking the same approach, that unless it's truncated or has more info don't show it. I don't think any are.

P.s good news, my favourite place in Wales isn't a result. 😉

Wales/llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch

[hayata-suenaga]

@dukenv0307 okay can you make a PR that removes the tooltip then? 🙇

[dukenv0307]

@dukenv0307 okay can you make a PR that removes the tooltip then? 🙇

@hayata-suenaga sure thing, can you assign me to the GH issue?

Thank you!

[trjExpensify]

I think you can use this GH, right? @peterdbarkerUK can align the expected results that we don't display tooltips on any of these values in the timezone picker.

[dukenv0307]

I think you can use this GH, right?

@trjExpensify yes I mean to assign me to this GH issue, I wasn't assigned yet 😄

[melvin-bot]

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

Offer link
Upwork job

[trjExpensify]

Cool, done!

[melvin-bot]

📣 @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]

@peterdbarkerUK, @jjcoffee, @hayata-suenaga, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

[dukenv0307]

@jjcoffee The PR is ready for review.

[hayata-suenaga]

The PR is still in review @jjcoffee just got back from OOO

[jjcoffee]

Still working on the PR, we're going to also fix #34156 on it.

[melvin-bot]

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

[melvin-bot]

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

• Remove tooltip of base section list #33580

If no regressions arise, payment will be issued on 2024-01-24. 🎊

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

• @jjcoffee requires payment automatic offer (Reviewer)
• @dukenv0307 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
• [@jjcoffee] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@jjcoffee] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@jjcoffee] Determine if we should create a regression test for this bug.
• [@jjcoffee] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@peterdbarkerUK] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: feat(selection-list): add tooltip #26091
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/26091/files#r1466113356
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A - just a missed case
• Determine if we should create a regression test for this bug. Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. Go to Settings -> Profile -> Timezone
2. Disable "automatically detect timezone" and go to timezone selector page
3. Hover over the timezone option
4. Verify that no tooltip is displayed
5. Go to FAB -> Start chat -> Room
6. Click on Visibility
7. Hover over the visibility option and verify that no tooltip is displayed

Do we agree 👍 or 👎

[jjcoffee]

@peterdbarkerUK Friendly bump for payment 🙇

[peterdbarkerUK]

Hmmm, not sure why this got stuck on weekly - I'm sorry @jjcoffee & @dukenv0307

Payment summary:
$500 @jjcoffee (Reviewer)
$500 @dukenv0307 (Contributor)

[peterdbarkerUK]

QA issue submitted https://github.com/Expensify/Expensify/issues/365429