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 - Text in compose box blends with background when theme was changed in device settings #39348

Closed izarutskaya closed 5 months ago

izarutskaya 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-4 Reproducible in staging?: Y Reproducible in production?: N Email or phone of affected tester (no customers): applausetester+omq23@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Precondition: user should be Signed In and "Use Device Settings" option should be selected for theme option in Preferences

Web, Desktop and mWeb steps:

  1. Open https://staging.new.expensify.com/

  2. Open any chat

  3. Navigate to device settings and change the theme to the opposite color

  4. Navigate back to browser or app

  5. Write any text in compose box

iOS steps:

  1. Open https://staging.new.expensify.com/

  2. Open any chat

  3. Write any text in the compose box

  4. Navigate to device settings and change the theme to the opposite color

  5. Navigate back to app

Expected Result:

Text should be properly displayed in compose box.

Actual Result:

The text in the compose box retains its original color despite changes to the background color. Consequently, it blends with the background when the device theme is switched to the opposite color.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/6ebf4449-e7a1-4ca3-9881-5f747cbc64ae

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b5c49154255020bb
  • Upwork Job ID: 1776299979557351424
  • Last Price Increase: 2024-04-05
melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 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.
izarutskaya commented 7 months ago

@anmurali 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.

izarutskaya commented 7 months ago

We think this issue might be related to the #vip-vsb.

izarutskaya commented 7 months ago

Production

https://github.com/Expensify/App/assets/115492554/c798f720-6e0b-41a7-9954-6ee9b4ffb099

MariaHCD commented 7 months ago

Able to reproduce the issue when switching to dark mode on staging web Chrome:

https://github.com/Expensify/App/assets/12268372/8461d51a-c1f8-4f63-be47-29f5e0fc63be

Wasn't able to repro on desktop, however.

melvin-bot[bot] commented 7 months ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 7 months ago

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

nkdengineer commented 7 months ago

I can raise the PR ASAP once assigned

Proposal

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

The text in the compose box retains its original color despite changes to the background color. Consequently, it blends with the background when the device theme is switched to the opposite color.

What is the root cause of that problem?

In here, we forgot to put flattenedStyle.color to the dependency list. So when the flattenedStyle.color changes due to theme changes, it's not reflected in updateTextColor, so the next time updateTextColor is run, it will update the text to the old color of the previous theme.

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

In here, put flattenedStyle.color to the dependency list.

What alternative solutions did you explore? (Optional)

NA

MariaHCD commented 7 months ago

Reassigning C+ for urgency: https://expensify.slack.com/archives/C02NK2DQWUX/p1711977525194839?thread_ts=1711977461.265819&cid=C02NK2DQWUX

melvin-bot[bot] commented 7 months ago

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

shubham1206agra commented 7 months ago

@BartoszGrajdek @tomekzaw @robertKozik Can you look at the above?

MariaHCD commented 7 months ago

Looks like this one can't be external since it pertains to the react-native-live-markdown repo

https://expensify.slack.com/archives/C02NK2DQWUX/p1711978269008509?thread_ts=1711977461.265819&cid=C02NK2DQWUX

nkdengineer commented 7 months ago

Looks like this one can't be external since it pertains to the react-native-live-markdown repo

@MariaHCD Sorry I don't have access to the link, can you explain why it can't be external since it pertains to the react-native-live-markdown repo?

Shouldn't we treat it like any other internal Expensify repos (like expensify-common, react-native-onyx, ...). Even if we consider it an external repo, contributors can still raise upstream PRs to fix it, like we did with all other upstream issues.

shubham1206agra commented 7 months ago

Looks like this one can't be external since it pertains to the react-native-live-markdown repo

@MariaHCD Sorry I don't have access to the link, can you explain why it can't be external since it pertains to the react-native-live-markdown repo?

Shouldn't we treat it like any other internal Expensify repos (like expensify-common, react-native-onyx, ...). Even if we consider it an external repo, contributors can still raise upstream PRs to fix it, like we did with all other upstream issues.

@nkdengineer We have not yet allowed external contributors to commit to the react-native-live-markdown. The commits will only be made by current maintainers.

nkdengineer commented 7 months ago

@nkdengineer We have not yet allowed external contributors to commit to the react-native-live-markdown.

@shubham1206agra Could you link to a discussion in GH/Slack which states this, or an official doc saying so (and the reason why it's decided as such if there is one)?

Thanks

shubham1206agra commented 7 months ago

@nkdengineer We have not yet allowed external contributors to commit to the react-native-live-markdown.

@shubham1206agra Could you link to a discussion in GH/Slack which states this, or an official doc saying so (and the reason why it's decided as such if there is one)?

Thanks

@nkdengineer Sorry, but I can't cause this requires me to dig up way older chats.

nkdengineer commented 7 months ago

@shubham1206agra No issue, I can ask again in the #expensify-open-source channel.

But earlier it seems my Slack access was accidentally removed, I've requested to access again via email, but so far no response yet.

@MariaHCD Could you help to ask someone that handles Slack invites, to extend an invite to me again? My email is nkdengineer@gmail.com

Appreciate it!

marcaaron commented 7 months ago

Bump everyone in this thread. What's the resolution here? I'm not sure based on the context left. Thanks!

MariaHCD commented 7 months ago

We've brought this to the attention of SWM since it looks like the fix is needed in the react-native-live-markdown repo.

I reported the same in the #react-native-live-markdown channel as well: https://expensify.slack.com/archives/C06BDSWLDPB/p1712032135082779

Essentially, I think this shouldn't block the app deploy and can be included in the tracking issue: https://github.com/Expensify/App/issues/36071

thienlnam commented 7 months ago

Added as a MED in the tracking issue

MariaHCD commented 7 months ago

Going to reassign this one to @thienlnam and @BartoszGrajdek

BartoszGrajdek commented 7 months ago

Hi! I'm Bartosz from SWM, let me take a look into it 👀

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

thienlnam commented 7 months ago

Opening this to external contributors!

nkdengineer commented 7 months ago

@hoangzinh FYI I already got a proposal above

tomekzaw commented 7 months ago

Web

The change described in the proposal has been independently applied in https://github.com/Expensify/react-native-live-markdown/pull/261 which has already been merged and released in react-native-live-markdown@0.1.37 (currently the App uses 0.1.38).

It seems like this bugs is already fixed on current main (please confirm).

iOS native

I was able to reproduce that bug earlier today, will investigate that.

hoangzinh commented 7 months ago

Based on this comment https://github.com/Expensify/App/issues/39348#issuecomment-2029838019, The react-native-live-markdown repo hasn't opened for external contributors yet. @thienlnam Is this issue still open for external contributors?

shubham1206agra commented 7 months ago

This is fixed on staging for the web. cc @thienlnam

nkdengineer commented 7 months ago

Based on this comment https://github.com/Expensify/App/issues/39348#issuecomment-2029838019, The react-native-live-markdown repo hasn't opened for external contributors yet. @thienlnam Is this issue still open for external contributors?

@hoangzinh Based on this thread, we've started to open up the react-native-live-markdown issues to external contributors, including this one.

shubham1206agra commented 7 months ago

Based on this comment #39348 (comment), The react-native-live-markdown repo hasn't opened for external contributors yet. @thienlnam Is this issue still open for external contributors?

@hoangzinh Based on this thread, we've started to open up the react-native-live-markdown issues to external contributors, including this one.

@nkdengineer You have misinterpreted the comment here.

nkdengineer commented 7 months ago

Based on this comment https://github.com/Expensify/App/issues/39348#issuecomment-2029838019, The react-native-live-markdown repo hasn't opened for external contributors yet. @thienlnam Is this issue still open for external contributors?

I think @thienlnam can help confirm here.

tomekzaw commented 7 months ago

I was able to reproduce the issue in Live Markdown example app, working on a fix for iOS native right now.

tomekzaw commented 7 months ago

Found the root cause in react-native-live-markdown, fix incoming

thienlnam commented 7 months ago

Sorry for the confusion - I have opened up some issues related this externally, but seems like @tomekzaw is already working on this and the fix requires changes in the original repo so I will be assigning to him

tomekzaw commented 7 months ago

Should be fixed with https://github.com/Expensify/react-native-live-markdown/pull/269.

I will submit a PR with version bump soon.

tomekzaw commented 7 months ago

Should be fixed with https://github.com/Expensify/App/pull/39872.

melvin-bot[bot] commented 6 months ago

This issue has not been updated in over 15 days. @hoangzinh, @tomekzaw, @thienlnam, @BartoszGrajdek 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!

BartoszGrajdek commented 6 months ago

@thienlnam can we get this tested & closed?

mallenexpensify commented 5 months ago

Added @anmurali back as BZ (not sure if they're needed though, we just need BZs added to all bugs, in case they are).

mvtglobally commented 5 months ago

Issue not reproducible during KI retests. (First week)

mallenexpensify commented 5 months ago

Closing, also unable to repro (and.. now I'm in dark mode!)

image