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-01-26] [$600] CRITICAL: Updates to Comment Action menu #33573

Closed
12 tasks
quinthar opened this issue Dec 25, 2023 · 40 comments
Closed
12 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@quinthar
Copy link
Contributor

quinthar commented Dec 25, 2023

Problem

As we increasingly build out our chat-focused features, the amount of available comment actions has increased pretty dramatically from a couple to ten and counting. It's pretty daunting to say the least. The comment action menu has become bloated and inconsistent over time.

Solution

  • Rename Subscribe to thread to Join thread

  • Rename Unsubscribe to thread to Leave thread

  • Rework the actions to only show 3 emojis, emoji reaction button, two actions, then overflow icon:
    image

  • Change Reply in thread icon to new comment_bubble_add icon

  • Change Mark as unread icon to new comment_bubble_unread icon

  • Adjust to new sizing removing the spacing between icons and sizing of them (see details below):
    image

  • Implement overflow menu for actions Add Expensify eslint #3 and beyond

Details about sizing

  • Button still 28x28px
  • Icons should be 18x18px
  • Emoji size should be 18x18px (or visually so)
  • Spacing between buttons should be 0px
  • Height of menu should be 36px
  • Menu padding should remain 4px

That should give us:

image
image
image
image
image

Realtime discussion in Slack here: https://expensify.slack.com/archives/C0671JDRKQW/p1703546061230429?thread_ts=1703546004.757449&cid=C0671JDRKQW

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017316d0730ce02286
  • Upwork Job ID: 1742747999864901632
  • Last Price Increase: 2024-01-26
  • Automatic offers:
    • esh-g | Contributor | 28080976
Issue OwnerCurrent Issue Owner: @muttmuure
@quinthar quinthar converted this from a draft issue Dec 25, 2023
@quinthar quinthar self-assigned this Dec 26, 2023
@melvin-bot melvin-bot bot added the Monthly KSv2 label Dec 29, 2023
Copy link

melvin-bot bot commented Jan 3, 2024

Triggered auto assignment to @NikkiWines (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@quinthar
Copy link
Contributor Author

quinthar commented Jan 3, 2024

I don't think @NikkiWines is needed; @dubielzyk-expensify can you update the original post of this to outline how you want to update the hover menu, and then tag with external + task + daily to get an external contributor assigned to it?

@esh-g
Copy link
Contributor

esh-g commented Jan 3, 2024

Proposal

Please re-state the problem

Rename "Subscribe" in action menu to "Join"

What is the root cause?

N/A (New feature)

What changes should be made to fix this?

The solution suggests three options. Here are possible implementations for all of them:

  1. Remove the 'subscribe to thread' and 'unsubscribe to thread' options entirely by removing them from the ContextMenuActions here:

    {
    isAnonymousAction: false,
    textTranslateKey: 'reportActionContextMenu.subscribeToThread',
    icon: Expensicons.Bell,

    {
    isAnonymousAction: false,
    textTranslateKey: 'reportActionContextMenu.unsubscribeFromThread',
    icon: Expensicons.BellSlash,

  2. Re-name the action to 'join thread'. This can be done by adding translation keys for reportActionContextMenu.joinThread and reportActionContextMenu.leaveThread and then using them in the places mentioned in point 1. We can also change the icon from Expensicons.Bell to Expensicons.ChatBubbles to mirror the room behaviour even more consistently.

  3. Re-name the action to 'follow thread'. This is similar to point 2 and can be done with the same process.

Additional change proposals

Option 1
Screenshot 2024-01-03 at 4 48 37 PM

The language slack uses is 'Get notified about new replies' which is very clear cut but very lengthy unfortunately for the Expensify app.
Screenshot 2024-01-03 at 4 49 46 PM

Option 2
What can be done also is use the getDescription property on the context menu action to have a description that explains the property so that there is no confusion for the user.

},
getDescription: () => {},
},

getDescription: () => Localize.translateLocal('reportActionContextMenu.joinThreadDescription'),
Screenshot 2024-01-03 at 4 58 43 PM

Or after changing the icon
Screenshot 2024-01-03 at 5 01 00 PM

For leaving thread
Screenshot 2024-01-03 at 5 04 51 PM

Additional overflow menu changes

  1. Reduce the overflow menu emojis to 3 by slicing CONST.QUICK_REACTIONS to 3 elements in MiniQuickEmojiReactions.js

    {_.map(CONST.QUICK_REACTIONS, (emoji) => (

  2. Modify the Icon for add emoji in MiniQuickEmojiReactions to be 18px:

  3. We should pass the props.isMini prop to ContextMenuItem.shouldShow method so that we can control which actions to show for the mini menu

  4. Add another context menu action (only for mini overflow menu) that calls the showContextMenu method in its onPress property and has it's icon as the Expensicons.ThreeDots.

  5. Modify the style of emoji to be 18px here:

    fontSize: 15,

  6. Add height: 36 and center using flex for MiniWrapperStyle here:

  7. Modify the reportActionContextMenuMiniButton to use flex centring and fix height and width as 28px

    reportActionContextMenuMiniButton: {

  8. Modify ContextMenuItem Icon to be 18px by passing height and width here:

Screen.Recording.2024-01-04.at.5.45.21.PM.mov

@melvin-bot melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Jan 3, 2024
@dubielzyk-expensify dubielzyk-expensify changed the title CRITICAL: Rename "Subscribe" in action menu to "Join" CRITICAL: Updates to Comment Action menu Jan 4, 2024
@dubielzyk-expensify dubielzyk-expensify added Task External Added to denote the issue can be worked on by a contributor labels Jan 4, 2024
@melvin-bot melvin-bot bot changed the title CRITICAL: Updates to Comment Action menu [$500] CRITICAL: Updates to Comment Action menu Jan 4, 2024
Copy link

melvin-bot bot commented Jan 4, 2024

Job added to Upwork: https://www.upwork.com/jobs/~017316d0730ce02286

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 4, 2024
Copy link

melvin-bot bot commented Jan 4, 2024

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

@dubielzyk-expensify
Copy link
Contributor

Description updated to reflect latest changes, @quinthar

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 4, 2024

This is a implementation based issue. Let's hire @esh-g for their proposal
🎀 👀 🎀

Copy link

melvin-bot bot commented Jan 4, 2024

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

@rushatgabhane
Copy link
Member

Additional change proposals

image

@esh-g this is smart but it kinda looks cluttered. Since this is a CRITICAL issue, let's raise a PR as soon as possible after assignment.

Meanwhile, @dubielzyk-expensify what do you think about adding description as proposed by @esh-g ?

@mountiny
Copy link
Contributor

Merged

Copy link

melvin-bot bot commented Jan 18, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 19, 2024
@melvin-bot melvin-bot bot changed the title [$500] CRITICAL: Updates to Comment Action menu [HOLD for payment 2024-01-26] [$500] CRITICAL: Updates to Comment Action menu Jan 19, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 19, 2024
Copy link

melvin-bot bot commented Jan 19, 2024

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

Copy link

melvin-bot bot commented Jan 19, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.27-1 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 2024-01-26. 🎊

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

@dubielzyk-expensify
Copy link
Contributor

Just realized that the unread icon isn't implemented:
CleanShot 2024-01-22 at 09 28 48@2x

The mail icon should be the new unread icon as outlined the description:
image

@esh-g
Copy link
Contributor

esh-g commented Jan 22, 2024

@dubielzyk-expensify Don't worry, I added it as a part of the follow-up PR for this issue here: #34754

@dubielzyk-expensify
Copy link
Contributor

Thanks!

@mountiny mountiny added NewFeature Something to build that is a new item. and removed Task labels Jan 26, 2024
Copy link

melvin-bot bot commented Jan 26, 2024

@mountiny mountiny changed the title [HOLD for payment 2024-01-26] [$500] CRITICAL: Updates to Comment Action menu [HOLD for payment 2024-01-26] [$600] CRITICAL: Updates to Comment Action menu Jan 26, 2024
Copy link

melvin-bot bot commented Jan 26, 2024

Upwork job price has been updated to $600

@mountiny
Copy link
Contributor

Payment is $600 for @esh-g @rushatgabhane see reasoning here

@mountiny
Copy link
Contributor

@muttmuure this is ready for payment now

@mountiny mountiny added Daily KSv2 and removed Weekly KSv2 labels Jan 31, 2024
@esh-g
Copy link
Contributor

esh-g commented Feb 3, 2024

gentle bump @muttmuure

@rushatgabhane
Copy link
Member

@JmillsExpensify
Copy link

$600 approved for @rushatgabhane based on comment above.

@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

Ready for payment @muttmuure

@muttmuure
Copy link
Contributor

Handling

@muttmuure
Copy link
Contributor

@esh-g paid $600 on Upwork

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 Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests

8 participants