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 2023-03-20] [$1000] Inconsistency while showing tooltip for Profile name and workspace name #15451

Closed
1 task done
kavimuru opened this issue Feb 23, 2023 · 35 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kavimuru
Copy link

kavimuru commented Feb 23, 2023

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


Action Performed:

  1. Go to new.expensify.com
  2. Navigate to Settings
  3. Hover over User's Avatar and you'll notice a tooltip showing User's name
  4. Navigate to Workspaces > Select any workspace
  5. Hover our Workspace's Avatar & you'll not get any tooltip

Expected Result:

Behavior should be consistent. (Either for Avatars or Names)

Actual Result:

Tooltip is showing for Avatar (Profile) but for Workspace it's shown for its name

Workaround:

unknown

Platforms:

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

  • MacOS / Chrome / Safari

Version Number: 1.2.76-3
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
Notes/Photos/Videos:

tooltips.mp4
Recording.1586.mp4

Expensify/Expensify Issue URL:
Issue reported by: @daraksha-dk
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1677163050183769

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015cf71b7f14fe90ac
  • Upwork Job ID: 1631091455908864000
  • Last Price Increase: 2023-03-02
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 23, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 23, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Feb 23, 2023

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 melvin-bot bot added the Overdue label Feb 27, 2023
@MelvinBot
Copy link

@MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@MitchExpensify
Copy link
Contributor

Will get to this tomorrow, sorry for the delay! ECX prep and ooo on Friday got in the way

@melvin-bot melvin-bot bot removed the Overdue label Mar 1, 2023
@MitchExpensify MitchExpensify added the External Added to denote the issue can be worked on by a contributor label Mar 2, 2023
@melvin-bot melvin-bot bot unlocked this conversation Mar 2, 2023
@melvin-bot melvin-bot bot changed the title Inconsistency while showing tooltip for Profile name and workspace name [$1000] Inconsistency while showing tooltip for Profile name and workspace name Mar 2, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~015cf71b7f14fe90ac

@MelvinBot
Copy link

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 2, 2023
@MelvinBot
Copy link

Triggered auto assignment to @youssef-lr (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@PrashantMangukiya
Copy link
Contributor

Proposal

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

For Profile it shows tooltip when mouse over on avatar but does not show for display name. For Workspace there is no tooltip for Avatar but it shows for workspace name. So there is Inconsistency while showing tooltip for Profile name and workspace name

What is the root cause of that problem?

Profile avatar wrapped with tooltip, but display name not. For workspace there is a tooltip wrapped on workspace name, but not for profile avatar.

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

To make consistent we have to decide that either have to keep tooltip on avatar or display name, or both. So accordingly we have to wrap missing tooltip on Profile and Workspace etc.

What alternative solutions did you explore? (Optional)

None.

@abdulrahuman5196
Copy link
Contributor

Proposal

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

Tooltip not shown consistently between profile settings page and workspace settings page

What is the root cause of that problem?

This is a straight forward feature which is not implemented. The places which is not showing Tooltip doesn't have ToolTip component implementation.

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

We need to add ToolTip implementation(Tooltip parent view component) to required places.
For example:
In settings page we are not showing Tooltip over name and email. We can add Tooltip implementation to it here

<Pressable style={[styles.mt1, styles.mw100]} onPress={this.openProfileSettings}>
<Text style={[styles.textHeadline]} numberOfLines={1}>
{this.props.currentUserPersonalDetails.displayName
? this.props.currentUserPersonalDetails.displayName
: Str.removeSMSDomain(this.props.session.email)}
</Text>
</Pressable>
{this.props.currentUserPersonalDetails.displayName && (
<Text
style={[styles.textLabelSupporting, styles.mt1]}
numberOfLines={1}
>
{Str.removeSMSDomain(this.props.session.email)}
</Text>
)}

In workspace page we are not showing Tooltip over image. We can add ToolTip implementation to the same
<View style={[styles.settingsPageBody, styles.alignItemsCenter]}>
<Pressable
disabled={this.hasPolicyCreationError()}
style={[styles.pRelative, styles.avatarLarge]}
onPress={this.openEditor}
>
{this.props.policy.avatar
? (
<Avatar
containerStyles={styles.avatarLarge}
imageStyles={[styles.avatarLarge, styles.alignSelfCenter]}
source={this.props.policy.avatar}
fallbackIcon={Expensicons.FallbackWorkspaceAvatar}
size={CONST.AVATAR_SIZE.LARGE}
/>
)
: (
<Icon
src={Expensicons.Workspace}
height={80}
width={80}
fill={themedefault.iconSuccessFill}
/>
)}
</Pressable>
similar to how profile page has implemented ToolTip
<Tooltip text={this.props.currentUserPersonalDetails.displayName}>

We can also use the same implementation in other places where tooltip is required to maintain consistency.

What alternative solutions did you explore? (Optional)

We can remove tooltip at some places if we feel it would make the tooltip redundant. We just have to remove the Tooltip component for the specific components which don't require tooltip.

@daraksha-dk
Copy link
Contributor

daraksha-dk commented Mar 2, 2023

Proposal

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

Inconsistency while showing tooltip for Profile name and workspace name

What is the root cause of that problem?

The root cause of this problem is using the tooltip for name at two different locations for User's Profile and Workspaces [Avatar & Name)

For Profile (Avatar)

<Tooltip text={this.props.currentUserPersonalDetails.displayName}>
<OfflineWithFeedback
pendingAction={lodashGet(this.props.currentUserPersonalDetails, 'pendingFields.avatar', null)}
>
<Avatar
imageStyles={[styles.avatarLarge]}
source={ReportUtils.getAvatar(this.props.currentUserPersonalDetails.avatar, this.props.session.email)}
size={CONST.AVATAR_SIZE.LARGE}
/>
</OfflineWithFeedback>
</Tooltip>

For Workspace (Name)

<Tooltip text={this.props.policy.name}>
<Text
numberOfLines={1}
style={[
styles.textHeadline,
styles.alignSelfCenter,
]}
>
{this.props.policy.name}
</Text>
</Tooltip>

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

In workspace, the tooltip shouldn't show the name of the workspace instead it should mention the action that we're going to take (same goes for User's profile page). In this case, tooltip text should be something like "General Settings" for Workspace and "Profile" for User.

One similar example:

image

As for the location of the tooltip(s), we currently have these two locations:

  • Name location => display name/policy name for User/Workspace
  • Avatar location => Avatar for User/Workspace

We need to decide if we want to have only one tooltip or two tooltips ( while keeping in mind that both Name & Avatar are currently pressable )

1. One tooltip (Text will be "General Settings" or "Profile")

Options

  • Show one single parent tooltip for both Name and Avatar combined
  • Show the tooltip at Name location only
  • Show the tooltip at Avatar location only

2. Two tooltips

Options

  • Show the same tooltips for both Avatar and Name locations ("General Settings" or "Profile")
  • Show 1 tooltip for "General Settings" at Avatar location & another "Name" tooltip at Name location
  • Show 1 tooltip for "General Settings" at Name location & another "Name" tooltip at Avatar location (Least preferred)

Lastly, in any case if we don't use the tooltip or use the "Name" tooltip at Name location then we can probably make it not pressable

My preference would be : 2b > 1a > 2a > 1c > 1b = 2c

What alternative solutions did you explore? (Optional)

@thesahindia
Copy link
Member

In workspace, the tooltip shouldn't show the name of the workspace instead it should mention the action that we're going to take (same goes for User's profile page). In this case, tooltip text should be something like "General Settings" for Workspace and "Profile" for User.

That makes sense to me. I had also suggested doing the same in the past.

I think we should ask @shawnborton to confirm the expected result. @shawnborton, can you look at this comment and share your thoughts?

@shawnborton
Copy link
Contributor

shawnborton commented Mar 2, 2023

Yeah, I would think that the tooltip over the avatar in Settings would say "Profile" since that's where clicking takes you. Then for the workspace avatar, I would think the tooltip would say "Workspace Settings" or "Workspace General Settings" or "General Settings" since that's where clicking it will take you. Is that the general consensus here?

@daraksha-dk
Copy link
Contributor

@shawnborton - I agree with your thoughts.
And could you also tell us whether we should keep dual tooltips for the same action or use a single tooltip.
I've mentioned all the options here

1. One tooltip (Text will be "General Settings" or "Profile")

Options

  • Show one single parent tooltip for both Name and Avatar combined
  • Show the tooltip at Name location only
  • Show the tooltip at Avatar location only

2. Two tooltips

Options

  • Show the same tooltips for both Avatar and Name locations ("General Settings" or "Profile")
  • Show 1 tooltip for "General Settings" at Avatar location & another "Name" tooltip at Name location
  • Show 1 tooltip for "General Settings" at Name location & another "Name" tooltip at Avatar location (Least preferred)

Lastly, in any case if we don't use the tooltip or use the "Name" tooltip at Name location then we can probably make it not pressable

My preference would be : 2b > 1a > 2a > 1c > 1b = 2c

@PrashantMangukiya
Copy link
Contributor

For this issue below is the expected and actual result statement.

Expected Result:
Behavior should be consistent. (Either for Avatars or Names)

Actual Result:
Tooltip is showing for Avatar (Profile) but for Workspace it's shown for its name

So I posted proposal above #15451 (comment) based on that.

I can see that another proposal #15451 (comment) comes with different approach (i.e. other than what mentioned in expected result in this issue). And now discussion going in different direction.

It will be ok if decide to go with different approach, but I will suggest to give priority to who posted proposal according to the issue expected and actual result statement.

Because anyone can suggest different approach in any issue, but we have to keep focus on what is the issue and what is expected result, and accept proposal accordingly.

cc: @mallenexpensify

@shawnborton
Copy link
Contributor

I agree that there should be two separate tooltips, as those are two separately tappable areas.

@thesahindia
Copy link
Member

@daraksha-dk's proposal is aligned with the requirements so I prefer their proposal.

Please make the final decision @youssef-lr

C+ reviewed 🎀👀🎀

@MitchExpensify
Copy link
Contributor

Invite sent for eventual C+ payment @thesahindia

Please apply to this Upwork Job once @youssef-lr gives you the green light @daraksha-dk: https://www.upwork.com/jobs/~015cf71b7f14fe90ac

Thank you!

@MelvinBot
Copy link

@youssef-lr, @MitchExpensify, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

@youssef-lr
Copy link
Contributor

@daraksha-dk's proposal looks good to me as well!

@melvin-bot melvin-bot bot removed the Overdue label Mar 6, 2023
@MitchExpensify
Copy link
Contributor

Nice! Invited you to the Upwork job @daraksha-dk 👍

https://www.upwork.com/jobs/~015cf71b7f14fe90ac

@daraksha-dk
Copy link
Contributor

Accepted the job, thank you.

@daraksha-dk
Copy link
Contributor

PR is ready @thesahindia.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Mar 13, 2023
@melvin-bot melvin-bot bot changed the title [$1000] Inconsistency while showing tooltip for Profile name and workspace name [HOLD for payment 2023-03-20] [$1000] Inconsistency while showing tooltip for Profile name and workspace name Mar 13, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 13, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Mar 13, 2023

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

If no regressions arise, payment will be issued on 2023-03-20. 🎊

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.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

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

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@MelvinBot
Copy link

MelvinBot commented Mar 13, 2023

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:

  • [@thesahindia / @youssef-lr] The PR that introduced the bug has been identified. Link to the PR: NA - This was introduced during implementation
  • [@thesahindia / @youssef-lr] 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: NA - This was introduced during implementation
  • [@thesahindia / @youssef-lr] 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: NA - This was introduced during implementation
  • [@MitchExpensify] Determine if we should create a regression test for the bug.
  • [@thesahindia] If we decide to create a regression test for the bug, please propose the regression test steps to the appropriate location to ensure the same bug will not reach production again.
  • [@MitchExpensify] Review the proposed regression test steps and location.
  • [@MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/271239

@MelvinBot
Copy link

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.

@MitchExpensify
Copy link
Contributor

Made a cal reminder for payment on 3/20

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Mar 19, 2023
@MitchExpensify
Copy link
Contributor

Paid @daraksha-dk with speed and reporting bonus
Paid @thesahindia with speed bonus

Contracts ended!

@melvin-bot melvin-bot bot removed the Overdue label Mar 21, 2023
@MitchExpensify
Copy link
Contributor

Bump on these steps @thesahindia / @youssef-lr

@thesahindia
Copy link
Member

It was implemented like this so I believe we should skip the first 3 steps.

Regression test proposal -

Initial settings page -

  1. Go to Settings
  2. Hover over the User's Avatar & display name and
  3. Verify the tooltip says "Profile"

Workspace settings page -

  1. Go to settings > Workspaces > Select any workspace
  2. Hover our Workspace's Avatar & workspace name
  3. Verify that a Tooltip will be shown saying "General Settings"

Do we agree 👍 or 👎

@MitchExpensify
Copy link
Contributor

Those regression steps look good to me @thesahindia !

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests