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.56k stars 2.9k forks source link

[$250] Chat - Size of text cursor is smaller when returning to any chat via browser back button #39303

Closed lanitochka17 closed 5 months ago

lanitochka17 commented 7 months 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: 1.4.58-1 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Start chat
  3. Create a group chat
  4. Split bill in the group chat
  5. Click on the split preview > Click any split member > Click Message
  6. In 1:1 DM, click on the IOU preview
  7. Click on the header subtitle to return to 1:1 DM
  8. Click browser back button until app returns to the group chat in Step 3

Expected Result:

The size of text cursor will remain the same when returning to group chat via browser back button

Actual Result:

The size of text cursor is smaller when returning to group chat via browser back button

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/04307fb8-cb63-4e23-b779-80bf212633d1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e868dd4a424eb130
  • Upwork Job ID: 1776300146755960832
  • Last Price Increase: 2024-04-05
  • Automatic offers:
    • paultsimura | Reviewer | 0
melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @marcaaron (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 7 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
lanitochka17 commented 7 months ago

We think that this bug might be related to #vip-vsp

lanitochka17 commented 7 months ago

@marcaaron 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

marcaaron commented 7 months ago

Seems possibly more related to markdown changes.

marcaaron commented 7 months ago

@thienlnam I asked for a re-test to see. It kinda feels like this would be broken everywhere (sorry if I end up wrong on that hunch).

thienlnam commented 7 months ago

Definitely possible - I'll add it to the main tracking list. But seems pretty harmless and I don't think needs to be a blocker

marcaaron commented 7 months ago

Cool. If you are not passionate about it. Then I'm gonna pop the label off.

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

marcaaron commented 7 months ago

p.s. confirmed with QA that this is not limited to Group Chats.

dylanexpensify commented 7 months ago

@quinthar heads up, I added this to vip-vsb, but with LOW given it's lack of impact on usability. LMK if you disagree!

melvin-bot[bot] commented 7 months ago

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

thienlnam commented 7 months ago

Opening this to external contributors!

melvin-bot[bot] commented 7 months ago

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

nkdengineer commented 7 months ago

Proposal

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

The size of text cursor is smaller when returning to group chat via browser back button

What is the root cause of that problem?

In here, we didn't check if the height is positive before returning the value. The height could be 0 if the parent element was not fully rendered in the DOM yet.

So on layout change, when we use the method to get the element height here to use as maxHeight, we're getting 0 as the height when returning to the chat via browser back button (and the composer container was not fully rendered in DOM yet), leading to the input height becomes very small and the cursor size is small.

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

In here, if the height is 0, we should return auto as the height. So in the next layout effect trigger, when the composer container has been rendered in DOM, it will have positive height and will show in the UI correctly.

return height ? `${height}px` : 'auto';

This is similar to the logic when numberOfLines is not defined here . There's a bug there also, the condition after || will never be triggered, it should be styles.height ? `${styles.height}px` : 'auto'.

What alternative solutions did you explore? (Optional)

NA

paultsimura commented 7 months ago

@BartoszGrajdek could you please check the @nkdengineer's proposal as it requires changes within react-native-live-markdown?

Both the suggested solution and the other found bug make sense to me.

BartoszGrajdek commented 7 months ago

I'll take a look at it later today! πŸ‘€

BartoszGrajdek commented 7 months ago

Makes sense to me! πŸ™ŒπŸ»

This is similar to the logic when numberOfLines is not defined here

Great catch! πŸ‘πŸ»

BartoszGrajdek commented 7 months ago

If @paultsimura decides to go with your proposal @nkdengineer you can make a PR directly to react-native-live-markdown and we'll try pushing it forward πŸ‘€

paultsimura commented 7 months ago

Awesome, then let's go with @nkdengineer's proposal πŸ™Œ

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

πŸ“£ @paultsimura πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 7 months ago

πŸ“£ @nkdengineer You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

nkdengineer commented 7 months ago

@BartoszGrajdek @paultsimura this PR is ready for preview.

melvin-bot[bot] commented 7 months ago

@paultsimura @thienlnam @dylanexpensify @nkdengineer this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

dylanexpensify commented 7 months ago

PR is merged Mel! Waiting for payment day

paultsimura commented 7 months ago

Waiting for payment day

I believe we still need a PR to bump the react-native-live-markdown version in the App, right @nkdengineer?

dylanexpensify commented 7 months ago

Ah nice, thanks @paultsimura! πŸ‘

nkdengineer commented 7 months ago

@paultsimura this PR is ready for preview.

paultsimura commented 6 months ago

This was deployed to production on April 22, due payment 2024-04-29

dylanexpensify commented 6 months ago

Payment summary:

Please apply or request!

BartoszGrajdek commented 6 months ago

Hi! Can we get this issue closed? πŸ˜„ I'm trying to clean up the Live Markdown tracking issue right now :)

thienlnam commented 6 months ago

We don't close issues until we get them paid out - but we can remove this from the tracking issue

EDIT: I went ahead and cleaned up the main tracking issue here https://github.com/Expensify/App/issues/36071

dylanexpensify commented 6 months ago

bump @nkdengineer!

nkdengineer commented 6 months ago

Hi @dylanexpensify, sorry for the delay, I've applied to the job https://www.upwork.com/jobs/~01e868dd4a424eb130

dylanexpensify commented 5 months ago

no worries, thanks!!

dylanexpensify commented 5 months ago

offer sent!

nkdengineer commented 5 months ago

Hi @dylanexpensify, this issue was created on Mar 30, and according to this announcement, issues created prior to the announcement date Apr 4 will remain the original price ($500).

If that's correct, could you help update the GH title and offers, thanks πŸ™‡

paultsimura commented 5 months ago

@dylanexpensify in addition to @nkdengineer's comment above, it was quite a complex task involving an upstream fix in the Live Markdown package, so that note makes sense to me. WDYT?

dylanexpensify commented 5 months ago

Ah, thanks for the catch, agree. Updated offer, and will pay out $500 in Upwork

paultsimura commented 5 months ago

I've just applied for the job as well, thanks.

dylanexpensify commented 5 months ago

sounds great!

paultsimura commented 5 months ago

@dylanexpensify what's the payout plan on this? The payment day was a month ago, 2024-04-29

dylanexpensify commented 5 months ago

@paultsimura paid last week! But just sent bonus along!

dylanexpensify commented 5 months ago

@nkdengineer waiting for you to accept offer!

nkdengineer commented 5 months ago

@dylanexpensify I accepted the offer. But the offer was for $250 so I guess a bonus of $250 will be sent too.

Thanks πŸ™‡

dylanexpensify commented 5 months ago

Correct! all done btw