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.53k stars 2.88k forks source link

[$250] mWeb - Room - Mention members list overlaps Send button #49474

Closed IuliiaHerets closed 2 weeks ago

IuliiaHerets commented 1 month 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!


Version Number: 9.0.38-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): gocemate+a2199@gmail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Create a room
  2. Type "@" into chat compose
  3. Verify member list opens

Expected Result:

Mention members list should not overlaps the Send button

Actual Result:

Mention members list overlaps Send button

Workaround:

Unknown

Platforms:

Screenshots/Videos

Bug6608624_1726736525054!mweb

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838693306849868080
  • Upwork Job ID: 1838693306849868080
  • Last Price Increase: 2024-09-24
  • Automatic offers:
    • dominictb | Contributor | 104208005
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 1 month ago

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

nyomanjyotisa commented 1 month ago

Proposal

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

Room - Mention members list overlaps Send button

What is the root cause of that problem?

We display the mention suggestion popup based on cursor coordinate here https://github.com/Expensify/App/blob/885d6f49aa922d372d6007b12fe768e4705a18a5/src/components/AutoCompleteSuggestions/index.tsx#L100

And the compose input has 8px paddingTop that makes the mention suggestion popup overlap with the compose input box image

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

add + 8 to the bottomValue to make it not overlap with the compose input

let bottomValue = windowHeight - (cursorCoordinates.y - scrollValue + y) - keyboardHeight + 8;

Might need add here to if needed

What alternative solutions did you explore? (Optional)

dominictb commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-19 16:22:54 UTC.

Proposal

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

Mention members list overlaps Send button

What is the root cause of that problem?

Nothing wrong with the bottomValue calculation. The RCA is here

https://github.com/Expensify/App/blob/885d6f49aa922d372d6007b12fe768e4705a18a5/src/components/AutoCompleteSuggestions/AutoCompleteSuggestionsPortal/index.tsx#L43

We minus the bottom with getBottomSuggestionPadding

If we remove this function, the suggestion looks like

Screenshot 2024-09-19 at 23 12 41

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

We should update this value

https://github.com/Expensify/App/blob/885d6f49aa922d372d6007b12fe768e4705a18a5/src/components/AutoCompleteSuggestions/AutoCompleteSuggestionsPortal/getBottomSuggestionPadding/index.ts#L2

to

  1. 0 (the result image is above)

  2. -6

    Screenshot 2024-09-19 at 23 14 04
  3. Keep the current value and update this line to

                <View style={StyleUtils.getBaseAutoCompleteSuggestionContainerStyle({left, width, bottom: bottom + getBottomSuggestionPadding()})}>{componentToRender}</View>

same as what we did here

What alternative solutions did you explore? (Optional)

We can adjust padding value depends on design team

dominictb commented 1 month ago

@johncschuster What do you think about my proposal?

melvin-bot[bot] commented 1 month ago

@johncschuster Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

parasharrajat commented 1 month ago

I think we need to adjust the bottom on mweb devices same as native. @dominictb's approach make sense to me. @dominictb Can you please check how does it look with this change?

dominictb commented 1 month ago

@parasharrajat

I think we need to adjust the bottom on mweb devices same as native.

So it's my third solution. Here's the result

https://github.com/user-attachments/assets/37fa376a-8521-4737-b1ba-97946fb3b40c

melvin-bot[bot] commented 1 month ago

@johncschuster, @parasharrajat Eep! 4 days overdue now. Issues have feelings too...

johncschuster commented 1 month ago

@parasharrajat can you take a look above?

parasharrajat commented 1 month ago

@dominictb's proposal looks good to me. Let's go with that approach. Make we we apply the same view line mobile native on mweb and keep the existing flow on web.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @dominictb 🎉 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[bot] commented 1 month ago

⚠️ 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.

BhuvaneshPatil commented 1 month ago

My Bad, instead of issue I used PR link.

dominictb commented 3 weeks ago

@johncschuster @techievivek @parasharrajat I just came across this thread and seems like the overlap between the suggestion box and composer/send button is expected. The fixed bottom padding is to keep the distance between the suggestion box and the cursor not too far away.

I think we should just close the PR and proceed payments since it is expected. Wdyt?

techievivek commented 3 weeks ago

Hmm, but I think we can fix the padding for this case, no?

techievivek commented 3 weeks ago

Let me bump the design team in the above thread to confirm if we are OK with keeping it as it is.

techievivek commented 2 weeks ago

Coming from here https://expensify.slack.com/archives/C01GTK53T8Q/p1729178050894439?thread_ts=1728373839.980369&cid=C01GTK53T8Q, this is expected behaviour for now so let's close the PR and finish up the payment, thanks.

dominictb commented 2 weeks ago

I closed the PR.

johncschuster commented 2 weeks ago

Payment Summary:

Contributor: @dominictb paid $250 via Upwork - PAID 🎉

Contributor+: @parasharrajat due $250 via NewDot

Upwork job here!