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-10-16] [$500] Feature Request: Add :emojicode: selectors when editing messages. #34442

Closed
1 of 6 tasks
m-natarajan opened this issue Jan 12, 2024 · 68 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Jan 12, 2024

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


Problem:

Add :emojicode: selectors when editing messages.

Solution:

As a NewDot user I have become accustomed to being able to express myself with emojis, and the default way I do that is by typing :emojicodes:. However, when editing an existing message, we have an emoji picker, but no support for :emojicodes:

Context/Examples/Screenshots/Notes:

Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @roryabraham
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1704991655069279

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b0abd6fa6e877b41
  • Upwork Job ID: 1745886924480851968
  • Last Price Increase: 2024-01-19
  • Automatic offers:
    • dukenv0307 | Contributor | 28120470
Issue OwnerCurrent Issue Owner: @twisterdotcom
@m-natarajan m-natarajan added Weekly KSv2 NewFeature Something to build that is a new item. labels Jan 12, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

@m-natarajan m-natarajan changed the title Add :emojicode: selectors when editing messages. Feature Request: Add :emojicode: selectors when editing messages. Jan 12, 2024
@neonbhai
Copy link
Contributor

Proposal

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

Add :emojicode: selectors when editing messages

What is the root cause of that problem?

New Feature

What changes do you think we should make in order

We use ComposerWithSuggestions for normal message input (which has emoji and mention suggestions),

but use the basic Composer for Editing messages.:

We should configure the ReportActionItemMessageEdit to use ComposerWithSuggestions

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 12, 2024
@stitesExpensify stitesExpensify self-assigned this Jan 12, 2024
@stitesExpensify stitesExpensify added 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 labels Jan 12, 2024
@melvin-bot melvin-bot bot changed the title Feature Request: Add :emojicode: selectors when editing messages. [$500] Feature Request: Add :emojicode: selectors when editing messages. Jan 12, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

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

Copy link

melvin-bot bot commented Jan 12, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jan 14, 2024
@dukenv0307
Copy link
Contributor

Proposal

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

Implement suggestion emojis and mention when editing message

What is the root cause of that problem?

New feaure

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

We only use Composer when editing message.

We can implement a new component like ComposerWithSuggestionsEdit. We can re-use some logic in ComposerWithSuggestions to implement this new component.

We shouldn't implement ComposerWithSuggestions for ReportActionItemMessageEdit because the global composer has many things different logic.

Here is the test branch: https://github.com/dukenv0307/App/tree/fix/34442

Optimize logic can be done in the PR

What alternative solutions did you explore? (Optional)

NA

Result

Screen.Recording.2024-01-15.at.12.38.09.mov

@laurenreidexpensify
Copy link
Contributor

Not overdue, Melvin being a bit harsh with the weekend clock

@melvin-bot melvin-bot bot removed the Overdue label Jan 15, 2024
@laurenreidexpensify
Copy link
Contributor

@eVoloshchak bump on review thanks

@eVoloshchak
Copy link
Contributor

I think we should proceed with @neonbhai's proposal
Using ComposerWithSuggestions is the cleaner approach, it will allow us to reuse an already existing logic
I do agree with the following concern from @dukenv0307 though

We shouldn't implement ComposerWithSuggestions for ReportActionItemMessageEdit because the global composer has many things different logic.

But I don't think creating a third component with overlapping logic would be a better solution. ComposerWithSuggestions ideally should just be a Composer with Suggestions component (that is not entirely the case now, the logic in ComposerWithSuggestions differs a little, but introducing a prop to control it in case we need to would be simpler)

🎀👀🎀 C+ reviewed!

Copy link

melvin-bot bot commented Jan 18, 2024

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

@dukenv0307
Copy link
Contributor

@eVoloshchak Actually, ComposerWithSuggestions is built for the global composer and it has many required props which don't have in ReportActionItemMessageEdit and some logic like focus of text input that doesn't same for composer of ReportActionItemMessageEdit.

that is not entirely the case now, the logic in ComposerWithSuggestions differs a little, but introducing a prop to control it in case we need to would be simpler

I believe that controlling ComposerWithSuggestions to use for edit message is very complex and it isn't simpler than create a new component as my suggestion. You can see my demo branch for more detail.

Copy link

melvin-bot bot commented Jan 19, 2024

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

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

next step: @stitesExpensify to assign

@stitesExpensify
Copy link
Contributor

@eVoloshchak does this change your opinion on which approach we should take? #34442 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Jan 23, 2024
@eVoloshchak
Copy link
Contributor

does this change your opinion on which approach we should take? #34442 (comment)

@stitesExpensify, it does. I took a look at @dukenv0307's test branch and ComposerWithSuggestions once again and I agree, there is too much logic that is specific to global composer, it would be simpler to just have a separate component. We should think of a better name for ComposerWithSuggestionsEdit on the PR stage

Let's proceed with @dukenv0307, sorry for the confusion folks

@laurenreidexpensify laurenreidexpensify added the NewFeature Something to build that is a new item. label Jun 20, 2024
Copy link

melvin-bot bot commented Jun 20, 2024

Triggered auto assignment to @twisterdotcom (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Jun 20, 2024

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

Copy link

melvin-bot bot commented Jun 20, 2024

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

@laurenreidexpensify
Copy link
Contributor

@twisterdotcom handing this one over for parental leave cover. We're still in review for the PR, so this will just be handling payment. We're also blocked on #41071 I believe. Thanks!

@laurenreidexpensify
Copy link
Contributor

(I also don't know why the automation decided this was ready for payment a few months ago, am updating now)

@laurenreidexpensify laurenreidexpensify removed the Awaiting Payment Auto-added when associated PR is deployed to production label Jun 20, 2024
@laurenreidexpensify laurenreidexpensify changed the title [HOLD for payment 2024-05-02] [$500] Feature Request: Add :emojicode: selectors when editing messages. [$500] Feature Request: Add :emojicode: selectors when editing messages. Jun 20, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jul 15, 2024
Copy link

melvin-bot bot commented Jul 15, 2024

This issue has not been updated in over 15 days. @eVoloshchak, @twisterdotcom, @stitesExpensify, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@eVoloshchak
Copy link
Contributor

This feature was added by another PR (#44720) already it turns out
There is one small UI issue (#41071 (review)), but otherwise, this is fully implemented

@laurenreidexpensify, @stitesExpensify, what would be the correct course of action here?
Should we at least fix this UI issue?

@twisterdotcom
Copy link
Contributor

We should fix the UI issue and pay out an acceptable amount for the work done up until now. I don't have a good sense of it, but I would say $500 might not be it. @eVoloshchak what's your honest assessment?

@eVoloshchak
Copy link
Contributor

This is a second PR (the first one contained a regression and had to be reverted), so the full payment would have been $250. Given that and the fact that the UI fix would be straightforward (just need to change a single padding number), $150 seems fair to me

@twisterdotcom
Copy link
Contributor

$150 it is. Payment Summary:

@dukenv0307
Copy link
Contributor

Thanks @twisterdotcom, I have accepted the offer on Upwork

Copy link

melvin-bot bot commented Oct 8, 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 Monthly KSv2 labels Oct 9, 2024
@melvin-bot melvin-bot bot changed the title [$500] Feature Request: Add :emojicode: selectors when editing messages. [HOLD for payment 2024-10-16] [$500] Feature Request: Add :emojicode: selectors when editing messages. Oct 9, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 9, 2024
Copy link

melvin-bot bot commented Oct 9, 2024

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

Copy link

melvin-bot bot commented Oct 9, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.46-5 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-10-16. 🎊

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

Copy link

melvin-bot bot commented Oct 9, 2024

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

  • [@eVoloshchak / @dukenv0307] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@twisterdotcom] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@twisterdotcom
Copy link
Contributor

I already did all the payment summary here.

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 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

9 participants