Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.5k stars 2.85k forks source link

[$2000] When you long-press on a message, the message moves out of the screen frame and is hidden #10632

Open mvtglobally opened 2 years ago

mvtglobally commented 2 years ago

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. Navigate to any chat with multiple messages
  2. Long press on the last message you sent

Expected Result:

When long pressing on the last message, we should adjust the scroll so that the message you just selected is visible.

Actual Result:

When you long-press on a message in iOS mobile, the message moves out of the screen frame and is hidden.

Workaround:

None

Platform:

Where is this issue occurring?

Version Number: 1.1.92-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

https://user-images.githubusercontent.com/43995119/187123528-d0427883-8364-40be-899d-4ba63c33d98d.MP4

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

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

@marcaaron Whoops! This issue is 2 days overdue. Let's get this updated quick!

marcaaron commented 2 years ago

User should be able to see which messages is being selected

What does this mean exactly... ? Looking at the video and I'm not even sure what the bug is.

marcaaron commented 2 years ago

When you long-press on a message in iOS mobile, the message moves out of the screen frame, and it’s not obvious which message you’ve selected

Also don't understand what is being proposed here.

roryabraham commented 2 years ago

The problem is that once the context menu pops up and the modal blur is applied, it's not clear which message you've long-pressed on. What I was imagining is that the message you selected would have a white background / be "immune" from the modal blur effect.

But I think a mockup would be very helpful here. @marcaaron suggested that we could use a "shimmer effect" as an alternative

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @michelle-thompson (Design), see these Stack Overflow questions for more details.

michelle-thompson commented 2 years ago

Hmm, what about when you hold down on a message the background highlights as grey like so? image

roryabraham commented 2 years ago

Yeah, that works! We'll also need to make sure that we:

roryabraham commented 2 years ago

Leaving you assigned so that you get the GH points @michelle-thompson

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

roryabraham commented 2 years ago

Sending this one to be external, keeping it a weekly for now

JmillsExpensify commented 2 years ago

Catching up on the above before exporting.

JmillsExpensify commented 2 years ago

Alright, had to think on this one a bit. I'm going to export it because I think it fits squarely with WhatsApp quality, specifically polish of existing functionality. (As a general rule, I think we should avoid adding any explicit new functionality while WhatsApp quality is in effect).

JmillsExpensify commented 2 years ago

Job created here: https://www.upwork.com/jobs/~01be1724c1619fb8dc

JmillsExpensify commented 2 years ago

Still waiting for proposals.

JmillsExpensify commented 2 years ago

Doubling the price to encourage proposals.

mollfpr commented 2 years ago

Scroll the chat view so that the highlighted chat is visible despite the context menu opening

If this means that we want to open the context menu of the last chat, we should make the chat visible rather than got covered by the context menu?

Make sure we un-highlight the option when the context menu closes

As you can see, we already have the highlighted chat when the context menu opened, and it's removed when the context menu opened. But the highlight looks pretty light so is not really visible behind the overlay of the context menu.

https://user-images.githubusercontent.com/25520267/195879027-a56c5d0b-e086-4732-bf91-97bc1464ec9f.mov

Santhosh-Sellavel commented 2 years ago

Thanks, @mollfpr.

@roryabraham @michelle-thompson Seems the highlight is already available, it's not visible clearly as @mollfpr said.

Scroll the chat view so that the highlighted chat is visible despite the context menu opening

If this means that we want to open the context menu of the last chat, we should make the chat visible rather than got covered by the context menu?

Interesting how are we gonna show context for this case?

roryabraham commented 2 years ago

If this means that we want to open the context menu of the last chat, we should make the chat visible rather than got covered by the context menu?

Correct. Good point – it might not be so much scrolling as pushing the whole ReportActionsView up to keep it in view. But let's imagine we make that change, and then you long-press on the top chat. Pushing it up would push it out of view, so you'd actually have to scroll up to keep it visible.

Seems the highlight is already available, it's not visible clearly as @mollfpr said.

Good point, I didn't notice this. Almost seems like we'd want the blurred overlay to happen everywhere except for the item you're highlighting. cc @michelle-thompson

Also seems like this could become a very complicated change ... If the bottom-docked menu pushes content up out of its way, then it's no longer a modal.

Santhosh-Sellavel commented 2 years ago

Also seems like this could become a very complicated change ... If the bottom-docked menu pushes content up out of its way, then it's no longer modal.

Yeah but this is how WhatsApp handles long presses on iOS IMG_CFD18AAEAA44-1

Just like Messages in iOS, blur out other messages.

https://user-images.githubusercontent.com/85645967/195909601-3c32273f-c9e0-450c-b176-41b7e9d667ea.MP4

But android it's a different story, they provide context options in the header

JmillsExpensify commented 2 years ago

Still working through the proposal and related solution.

JmillsExpensify commented 2 years ago

It doesn't look like we've made much movement on this one in the last week. @roryabraham @Santhosh-Sellavel - I'm going to double the price of this issue as a result.

That said, I also want to come back to this comment from @roryabraham.

Also seems like this could become a very complicated change ... If the bottom-docked menu pushes content up out of its way, then it's no longer a modal.

Let's align on our best approach. Perhaps we should more appropriately pause this issue, or at the very least, consider this a new feature / polish rather than a bug. If that's the case, I'd argue we should take this back to Slack and align on the intended behavior before moving forward with said feature/polish.

Thoughts?

Santhosh-Sellavel commented 1 year ago

Still looking for proposals!

JmillsExpensify commented 1 year ago

Before we circle back on proposals, I think we should align on my last comment, specifically this one

Perhaps we should more appropriately pause this issue, or at the very least, consider this a new feature / polish rather than a bug. If that's the case, I'd argue we should take this back to Slack and align on the intended behavior before moving forward with said feature/polish.

Santhosh-Sellavel commented 1 year ago

Let's do that @JmillsExpensify Remove Help Wanted, Bug labels. Please apply the New Feature Label, and also apply the design label!

Santhosh-Sellavel commented 1 year ago

I just saw @shawnborton is already assigned here!

JmillsExpensify commented 1 year ago

Cool, I'm also putting this on hold given that it's not a roadmap priority.

JmillsExpensify commented 1 year ago

Quick update on this thread, as I just got on a call with @roryabraham and we aligned on what's happened here. Basically this comes down to two separate issues:

So we should align on what the best next steps are now that this is hopefully clear to all, or minimally to me. 😄

JmillsExpensify commented 1 year ago

I tend to think we should solve the bug first and foremost. However, in solving the bug I could also see the argument that we should also solve the highlight too.

roryabraham commented 1 year ago

Since we agree that there is a bug here, should we remove the [HOLD] and switch it from New Feature to Bug?

roryabraham commented 1 year ago

Also, maybe we should separate the two concerns into separate issues? And put the Polish on HOLD for the bug fix

JmillsExpensify commented 1 year ago

I'm not super passionate about whether we solve these separately or together. If we're just improving the color we use to highlight the message, then we could add both in one and raise the price. Does anyone have a strong preference one way or the other?

melvin-bot[bot] commented 1 year ago

Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new.

JmillsExpensify commented 1 year ago

Went ahead and removed the hold! I also updated this issue with a more clear description of the bug. I'll circle back on the polish issue, not a priority at this very moment.

Santhosh-Sellavel commented 1 year ago

Recently came off hold, open for proposals or not @JmillsExpensify?

JmillsExpensify commented 1 year ago

Yes! We're open for proposals. Upwork job is here: https://www.upwork.com/jobs/~01be1724c1619fb8dc. Kicking is off at the last price, which is also the new starting price for bugs.

Santhosh-Sellavel commented 1 year ago

not overdue, looking fir proposals

JmillsExpensify commented 1 year ago

Still waiting on proposals. I'll swing back for a price increase next week.

roryabraham commented 1 year ago

Still waiting for proposals, brought up in slack here

JmillsExpensify commented 1 year ago

Doubling the price in the meantime.

JmillsExpensify commented 1 year ago

Note: The previous upwork job expired, so the new one is here. https://www.upwork.com/jobs/~01c649adebe4e80002

arosiclair commented 1 year ago

I can take this internal if you guys don't mind @roryabraham

Santhosh-Sellavel commented 1 year ago

I believe it will be 👍 only @arosiclair, please take it. Remove the Help wanted label and assign yourself. Keep me assigned I'll help with the PR review, thanks!

roryabraham commented 1 year ago

Thanks @arosiclair, I'd like to review the PR when it's ready

arosiclair commented 1 year ago

I repro'd this in dev on iOS, Android and mWeb.

After thinking about this a bit more I'd have to echo Rory's sentiment about the complexity of fixing this. The context menu is a modal so it intentionally covers the conversation view. We could try scrolling the selected message up to the top so it doesn't get covered, but it would still be blurred and messages at the bottom of the conversation have no more room to scroll.

I think the only true way to fix this is to add a preview of the selected message to the menu. Something along the lines of this:

Adding that is squarely new feature/polish territory though and maybe could use some design input. What do you think @JmillsExpensify?

JmillsExpensify commented 1 year ago

Ah that's interesting. That's one of the potential solutiosn that @roryabraham and I discussed over the phone when we aligned earlier. That would also put us back where we were before, which to echo you is New Feature territory. At that point I think we need to put this feature on HOLD because it's not a WAQ priority.

JmillsExpensify commented 1 year ago

@roryabraham As the issue reporter, I get the sense that you're pretty passionate about this one. Given that we're struggled with bug/feature a couple of times in this issue – and more recently, might fix the "bug" by building a new feature – I'm back to feeling that this isn't really a priority right now. Curious for your thoughts!

roryabraham commented 1 year ago

I unfortunately am not a fan of @arosiclair's proposed solution. I think that ultimately the solution to this is a big lift which changes the context menu such that it's no longer a modal but a menu that slides in from the bottom of the screen and pushes content up out of its way. It sucks that it's a big lift, but just because it's hard to do doesn't make it less of a priority. I see this as a glaring usability problem with the app so definitely wouldn't deprioritize