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.

Sign up for GitHub

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

Jump to bottom

$1000 - When you edit a comment, the composer stays hidden on other chats - reported by @thesahindia #8031

Closed
mvtglobally opened this issue Mar 8, 2022 · 77 comments
Closed

$1000 - When you edit a comment, the composer stays hidden on other chats - reported by @thesahindia #8031

mvtglobally opened this issue Mar 8, 2022 · 77 comments
Assignees
@Beamanator @parasharrajat @marcaaron @MitchExpensify
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@mvtglobally
Copy link

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. Open :expensify: on Mobile
  2. Navigate to a chat
  3. Edit a comment
  4. Navigate to a different chat

Expected Result:

Composer should be visible on other chats or user shouldn't be able to navigate to different chat until he cancels/Save the changes

Actual Result:

Composer is hidden for every chat

Workaround:

unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android
  • Mobile Web

Version Number: 1.1.41-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2022-02-24.at.3.53.48.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @thesahindia
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1645699620022559

View all open jobs on GitHub
@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Mar 8, 2022
@MelvinBot
Copy link

Triggered auto assignment to @laurenreidexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 8, 2022
@laurenreidexpensify laurenreidexpensify changed the title When you edit a comment the composer stays hidden on other chats - reported by @thesahindia When you edit a comment, the composer stays hidden on other chats - reported by @thesahindia Mar 8, 2022
@laurenreidexpensify
Copy link
Contributor

@thesahindia I'm not sure I follow full here - what do you mean by "edit a comment". Do you mean editing a message that has already been sent?

@thesahindia
Copy link
Member

Do you mean editing a message that has already been sent?

Yes.

@laurenreidexpensify
Copy link
Contributor

Ah sorry I was a bit slow! Okay I've just replicated this on mobile now, let's get engineering to confirm it's okay to be external and get someone on it!

@MelvinBot
Copy link

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

@robertjchen
Copy link
Contributor

robertjchen commented Mar 9, 2022 •
edited
Loading

Composer should be visible on other chats or user shouldn't be able to navigate to different chat until he cancels/Save the changes

Got it! So to clarify my understanding, what should be the proper behavior here?

At the moment, the composer disappears after switching to another chat while in edit mode. I see three options to make it better:

  1. Automatically cancel the edit and restore the composer when switching a chat
  2. Composer should remain visible on other chats, but switching back to the old chat will resume the edit
  3. Prompt the user to save/cancel the chat (e.g. highlight composer box in red and prevent user from switching, a dialog, etc.)

I like 2 and 1, in that order. Any other thoughts on the best option moving forward?

@robertjchen
Copy link
Contributor

@thesahindia Could you please confirm which option we're intending to go with in order to address this behavior? Thanks!

@MelvinBot MelvinBot removed the Overdue label Mar 14, 2022
@thesahindia
Copy link
Member

I like the 2nd option. 1st option is good but there is a drawback. If the user is in the middle of editing and navigates to some other chat then the changes he made will get discarded.

@robertjchen
Copy link
Contributor

Got it, that makes sense- it should mirror the behavior on the desktop version (which is working properly, just not on mobile). This is clear to me now, apologies for any initial confusion!

So, the report/proposal here is to address the issue where the composer disappears when switching to other chats on mobile 👍 Thanks again for the report- going to add the External label for the next step of the process.

@robertjchen robertjchen removed their assignment Mar 15, 2022
@robertjchen robertjchen added the External Added to denote the issue can be worked on by a contributor label Mar 15, 2022
@MelvinBot
Copy link

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

@MitchExpensify
Copy link
Contributor

Upwork Job

@botify botify removed the Daily KSv2 label Mar 15, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Mar 15, 2022
@melvin-bot melvin-bot bot removed the Overdue label May 11, 2022
@Beamanator
Copy link
Contributor

Ping'd the slack channel again to see if anyone can help come to a final decision :D

@parasharrajat
Copy link
Member

Bump! Any update?

@Beamanator
Copy link
Contributor

I'm getting fewer and fewer responses to my posts in slack 😅 I don't feel comfortable moving forward till we have more feedback on the thought so I will tag CMEs and try to get more thoughts

@MitchExpensify I think you can hold off doubling this time please :D Since we don't have a solid way forward yet

@marcaaron
Copy link
Contributor

Heads up I'm working on this internally in a duplicate issue here and found a pretty simple solution.

@marcaaron
Copy link
Contributor

I think the correct behavior is that we should hide and show the "composer" only when the edit comment field is focused or blurred.

@marcaaron
Copy link
Contributor

Has anyone been hired? If not, I think we can just close this one (and the Upwork job) and have @parasharrajat and @Beamanator test my PR linked to the duplicate issue.

@Beamanator
Copy link
Contributor

@marcaaron I ended up hiring @parasharrajat here: #8031 (comment)

After that, we've been discussing what to do in some other situations (see Slack thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1652268683129369?thread_ts=1647881926.785649&cid=C01GTK53T8Q)

And this has been delayed due to lack of responses, which is why I just brought this to an internal slack channel as well

@Beamanator
Copy link
Contributor

If you already have a PR up, maybe we can pay @parasharrajat to help test & review it? (I'll help as well)

@parasharrajat
Copy link
Member

What is decided here? Should I close my PR?

@marcaaron
Copy link
Contributor

Yes, want to help us review this solution instead --> #9060

@marcaaron
Copy link
Contributor

Added you guys there.

@marcaaron marcaaron self-assigned this May 25, 2022
@marcaaron marcaaron added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels May 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 25, 2022

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

@melvin-bot melvin-bot bot added the Overdue label Jun 2, 2022
@Beamanator
Copy link
Contributor

Not overdue, PR testing and reviewing in progress

@melvin-bot melvin-bot bot removed the Overdue label Jun 2, 2022
@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2022
@Beamanator
Copy link
Contributor

I believe this can be closed b/c the fix was implemented in this PR: #9060

@melvin-bot melvin-bot bot removed the Overdue label Jun 10, 2022
@MitchExpensify
Copy link
Contributor

Agree!

@parasharrajat
Copy link
Member

parasharrajat commented Jun 14, 2022 •
edited
Loading

Note: I was hired for this issue here and It was decided that this issue will be solved internally and I will review it as C+. Also, I closed my PR for the same.

cc: @MitchExpensify

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Jun 15, 2022 •
edited
Loading

Ok so a consequence of that is you being owed C+ payment once it has gone 7 days without regression, is that right @parasharrajat ?

@parasharrajat
Copy link
Member

parasharrajat commented Jun 15, 2022 •
edited
Loading

Yeah, you are correct.

@thesahindia
Copy link
Member

@MitchExpensify, the reporting compensation is also pending.

@MitchExpensify
Copy link
Contributor

Thanks for the heads up team! I have made a note to issue payment tomorrow as that will be 7 days since the fix was deployed here

@MitchExpensify
Copy link
Contributor

Payments made and contract ended - Thanks @parasharrajat & @thesahindia!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Beamanator Beamanator

@parasharrajat parasharrajat

@marcaaron marcaaron

@MitchExpensify MitchExpensify

Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests