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

[$500] Details - Cursor is not pointer when hovered over the Avatar in workspace invite message page #29456

Closed
2 of 6 tasks
kbecciv opened this issue Oct 12, 2023 · 28 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Oct 12, 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!


Version Number: 1.3.81.6
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: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697038101560199

Action Performed:

  1. Create a group with some users and go to group details.
  2. Observe hovering on avatar has pointer cursor.
  3. Create a workspace if haven't already,
  4. Navigate to Invite Members in workspace
  5. Invite some some members Click Next
  6. Hover over the avatar for users

Expected Result:

Cursor should be pointer when hovered over the Avatar

Actual Result:

Cursor is not Pointer

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

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2023-10-11.at.8.46.22.PM.mov
MacOS: Desktop
Screen.Recording.2023-10-11.at.8.46.22.PM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cf3ed02adac008de
  • Upwork Job ID: 1712483295213932544
  • Last Price Increase: 2023-10-12
  • Automatic offers:
    • akinwale | Reviewer | 27231740
    • dukenv0307 | Contributor | 27231741
    • ishpaul777 | Reporter | 27231743
@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 Oct 12, 2023
@melvin-bot melvin-bot bot changed the title Details - Cursor is not pointer when hovered over the Avatar in workspace invite message page [$500] Details - Cursor is not pointer when hovered over the Avatar in workspace invite message page Oct 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 12, 2023

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

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

melvin-bot bot commented Oct 12, 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
Copy link

melvin-bot bot commented Oct 12, 2023

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

@abdel-h66
Copy link
Contributor

Proposal

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

Inconsistent cursor when hovering over the users avatars in the invite message page

What is the root cause of that problem?

The MultipleAvatars component is usually used withing a Pressable, which automatically applies a cursor:pointer. but in this case, we are wrapping the MultipleAvatars with a View component that does not give it the wanted cursor:pointer styling

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

We can add styles.cursorPointer to wrapper View

<View style={[styles.mv4, styles.justifyContentCenter, styles.alignItemsCenter]}>

What alternative solutions did you explore? (Optional)

N/A

@ishpaul777
Copy link
Contributor

ishpaul777 commented Oct 12, 2023

Proposal

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

Inconsistent cursor when hovering over the users avatars in the invite message page

What is the root cause of that problem?

The MultipleAvatars component we are using in for the workspace dont have a cursor: pointer style spplied to it

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

We can safely remove the wrapper view and can add a new prop avatarContainerStyle for passing all necessary style along with cursor pointer.

Alternative

Currently we are not allowing the user to see avatar of the users in full view attachment modal in invite message page it would be a great addition to UX if we they can check out the member avatar in full View Attachement modal, we can use RoomHeaderAvatars component instead for MultipleAvatar

@dukenv0307
Copy link
Contributor

Proposal

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

Cursor is not pointer when hovered over the Avatar in workspace invite message page

What is the root cause of that problem?

Cursor is not pointer because the Avatar in workspace invite message page is currently not clickable. Meanwhile the ones in report details page is clickable (to view the full avatar in the modal)

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

We should add the ability to view avatar in full in a modal to MultipleAvatars, by adding a properly like allowAvatarFullView, defaulting to false.

If that prop is true, we nest the Avatar inside an AttachmentModal like in here, and open the modal once the avatar is clicked.

Then we can just switch that property to true here and in any other pages that this issue is occurring, there're many pages that use MultipleAvatars as far as I can see.

What alternative solutions did you explore? (Optional)

We can also make the allowAvatarFullView a property of the Avatar component itself, which means the AttachmentModal logic will be inside Avatar, and the MultipleAvatars will just pass the prop to its Avatar.

@bernhardoj
Copy link
Contributor

The avatar is not clickable, so the cursor is already correct. Not a bug.

@akinwale
Copy link
Contributor

After reviewing the proposals, the solution in @dukenv0307's proposal is the most complete as it would properly fix the issue and add functionality that would maintain consistency with other parts of the app, if the issue is considered worth fixing.

🎀 👀 🎀 C+ reviewed.

@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

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

@garrettmknight
Copy link
Contributor

Validated this is still happening and I do think this is an inconsistency.

@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2023
@chiragsalian
Copy link
Contributor

Proposal LGTM, feel free to create a PR @dukenv0307.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

📣 @akinwale 🎉 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

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

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

melvin-bot bot commented Oct 17, 2023

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

Offer link
Upwork job

@dukenv0307
Copy link
Contributor

@akinwale @chiragsalian I want to double-confirm the places that we want to apply the change in my proposal

  1. Should we only want to wrap the avatars in an attachment modal when the avatars are displayed horizontally like this
Screenshot 2023-10-17 at 16 03 59
  1. Apply change to all avatars, including oblique avatars like this:
Screenshot 2023-10-17 at 16 05 43

@akinwale
Copy link
Contributor

@dukenv0307 I believe the main goal is to maintain consistency across the app, so if there are other parts of the app where avatars are clickable as identified in case 2 (oblique avatars), then you should apply that change in your PR too. Otherwise, let's just stick with case 1.

@garrettmknight can also share some thoughts on this.

@akinwale
Copy link
Contributor

@chiragsalian @garrettmknight bump for feedback to this comment. Thanks.

@chiragsalian
Copy link
Contributor

hmm oops i'm sorry i think i might have steered this in a wrong direction. I originally thought that those avatars were clickable and hence we wanted to show pointer icon. I see now that they weren't clickable to begin with so not showing pointer is correct. Cause if we show pointer icon and the user attempts to click it and nothing happens showing the pointer is bad user feedback.

My new thoughts:

  1. I do not think we should make avatar clickable throughout the app to show user details, at least not yet and that would be a much larger discussion if we did want to implement that.
  2. I think if we only wanted avatars in workspace to be clickable to show user details that's fine with me, but even that is a new feature. I don't think this new feature is really needed right now.

So i think next steps is do nothing and close this out.
@garrettmknight let me know if you disagree. I think the behavior is working as expected i.e., pointer only for clickable items and the avatar here is not clickable.

@akinwale
Copy link
Contributor

So i think next steps is do nothing and close this out. @garrettmknight let me know if you disagree. I think the behavior is working as expected i.e., pointer only for clickable items and the avatar here is not clickable.

@chiragsalian I think we can we put this on hold while we have a broader discussion about the expected behaviour. I can also start a discussion on Slack for this. What do you reckon?

@chiragsalian
Copy link
Contributor

yes looks like garrett hasn't had a chance to check this out yet. Sure yes can you begin a discussion on slack.

@melvin-bot melvin-bot bot added the Overdue label Oct 23, 2023
@akinwale
Copy link
Contributor

Started a Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1698059982808569

@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@garrettmknight
Copy link
Contributor

Commented as much in the thread (thanks for starting it @akinwale), but I agree with @chiragsalian that the behavior that exists is expected. Closing this one, sorry it took a little while!

@dukenv0307
Copy link
Contributor

dukenv0307 commented Oct 30, 2023

@garrettmknight @chiragsalian Should contributors be eligible for compensation in this issue. The contributor was assigned and I also spent a lot of time to implement the solution and just wait for the discussion to open the PR.

There were many precedents to this like #20674 (comment) and #28844 (comment) where compensation is eligible

@chiragsalian
Copy link
Contributor

I'll let garrett handle the payment and decision. Personally, i think no because we normally pay for merged code. A person can spend days on a PR but if it's not merged we don't normally pay them despite the amount of effort put into it. With that said there are exceptions so I'll let garrett decide this.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Nov 1, 2023

There were many precedents to this like #20674 (comment) and #28844 (comment) where compensation is eligible

Thanks for looking into this! @garrettmknight just to note in other similar cases above, full compensation is eligible, especially when the reason the PR is not merged is outside of control of the assigned contributors. We couldn't have known (or can do anything about) that the requirement will change after being assigned.

@garrettmknight
Copy link
Contributor

Hey @dukenv0307 - there are issues where we don't merge the code and pay for the work done when it's significant. In this case, we didn't have an approved PR or even a PR up and submitted so I don't think it meets the bar for full or partial payment.

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants