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.51k stars 2.87k forks source link

[HOLD][$250] Android - Chat - Sending code block message with italic is not applied& preview inconsistent #39623

Open lanitochka17 opened 7 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.60 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4478250 Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Tap on a report
  3. Enter |||
  4. Launch app
  5. Tap on a report
  6. Enter |||
  7. Note the difference in preview in mweb and app
  8. Send the message
  9. Note italic markdown not applied

Expected Result:

Code block with italic preview must not be inconsistent in mweb and Android. Sending code block message with italic must be applied in Android

Actual Result:

Code block with italic preview is inconsistent in mweb and Android. Sending code block message with italic is not applied in Android

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/03504c85-9b50-4416-8f86-024f7bdaee3e

View all open jobs on GitHub

melvin-bot[bot] commented 7 months ago

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

lanitochka17 commented 7 months ago

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

lanitochka17 commented 7 months ago

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

bernhardoj commented 7 months ago

Edited by proposal-police: This proposal was edited at 2024-10-02 16:04:19 UTC.

Proposal

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

Italic/bold isn't applied to inline code block.

What is the root cause of that problem?

The font-weight and style here are always set to undefined. https://github.com/Expensify/App/blob/837152e3df81ebecb2e1166f4f2ddf79ef968674/src/components/HTMLEngineProvider/HTMLRenderers/CodeRenderer.tsx#L28-L37

But even after we remove it, the issue still persists. That's because the MONOSPACE style is set for code, https://github.com/Expensify/App/blob/837152e3df81ebecb2e1166f4f2ddf79ef968674/src/styles/index.ts#L201-L206

and the style has font style and weight as normal. https://github.com/Expensify/App/blob/837152e3df81ebecb2e1166f4f2ddf79ef968674/src/styles/utils/FontUtils/fontFamily/singleFontFamily/index.ts#L13-L17

But it is still not enough because we don't have the italic and bold italic variants for ExpensiMono. We only have ExpensiMono-Regular and -Bold.

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

Remove the font style and weight override here. https://github.com/Expensify/App/blob/837152e3df81ebecb2e1166f4f2ddf79ef968674/src/components/HTMLEngineProvider/HTMLRenderers/CodeRenderer.tsx#L28-L37

Then, only take the font family style from MONOSPACE.fontFamily https://github.com/Expensify/App/blob/837152e3df81ebecb2e1166f4f2ddf79ef968674/src/styles/index.ts#L206

Then, add a new font asset for ExpensiMono-Bold and ExpensiMono-BoldItalic.

There is a docs regarding adding a new font.

Example where the font has regular, bold, italic, and bold italic variant. https://github.com/Expensify/App/blob/1764d104e4f1c3f0440a5425d5fe0f540584866b/android/app/src/main/res/font/expensify_neue.xml#L2-L6

melvin-bot[bot] commented 6 months ago

@isabelastisser Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

hoangzinh commented 6 months ago

Thanks for your proposal @bernhardoj. There are 2 things that is not clear in your proposal:

bernhardoj commented 6 months ago

Why does code block messages with italic is applied in Android mWeb but not native?

The web can inherits the italic style from the tag.

Screenshot 2024-04-13 at 13 59 48

But native can't.

Screenshot 2024-04-13 at 14 17 07

How would we solve the inconsistent issue of live markdown of Code block with italic between mweb and Android?

I think we need help from SWM since it needs changes on the native code to conditionally apply different font family if it's italic/bold

isabelastisser commented 6 months ago

Not overdue.

hoangzinh commented 6 months ago

Discussing with @isabelastisser internally here https://expensify.slack.com/archives/C02NK2DQWUX/p1713270353215539

hoangzinh commented 6 months ago

I think we need help from SWM since it needs changes on the native code to conditionally apply different font family if it's italic/bold

Hi @bernhardoj if we allow external contributors to contribute to react-native-live-markdown. Are you able to fix this issue?

bernhardoj commented 6 months ago

@hoangzinh I will try to look if I can fix it in react-native-live-markdown too.

hoangzinh commented 6 months ago

Thanks @bernhardoj . @tomekzaw just wanna confirm if we've opened react-native-live-markdown for external contributors

tomekzaw commented 6 months ago

For the reference, here's how it currently looks across platforms (checked on 83d1166b51):

Web:

Screenshot 2024-04-17 at 12 11 57

Android:

Screenshot 2024-04-17 at 12 12 54

iOS:

Screenshot 2024-04-17 at 12 14 38

Q: What's the expected behavior? Should the outside formatting (bold/italic) be reflected in inline code?

Also, the problem is not only related to Live Markdown Preview (in the composer box) but also in chat history (report item) so it will also require changes in E/App.

FYI Slack does support nesting styles for inline code:

Screenshot 2024-04-17 at 12 17 35

just wanna confirm if we've opened react-native-live-markdown for external contributors

I think this issue can be made external, what do you think? @thienlnam

hoangzinh commented 6 months ago

Q: What's the expected behavior? Should the outside formatting (bold/italic) be reflected in inline code?

I'm inclined to "Web". What do you think? @isabelastisser

melvin-bot[bot] commented 6 months ago

@hoangzinh @isabelastisser 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!

melvin-bot[bot] commented 6 months ago

@hoangzinh, @isabelastisser Huh... This is 4 days overdue. Who can take care of this?

hoangzinh commented 6 months ago

@isabelastisser When you have time, could you check this comment https://github.com/Expensify/App/issues/39623#issuecomment-2060928016? TIA

isabelastisser commented 6 months ago

@hoangzinh I agree, let's move forward with this. Thanks!

hoangzinh commented 6 months ago

@bernhardoj we agreed to allow external contributors to repo react-native-live-markdown for this issue. Are you able to update your proposal again with this https://github.com/Expensify/App/issues/39623#issuecomment-2061327690? Thanks

isabelastisser commented 6 months ago

Bump @bernhardoj on the comment above. Thanks!

bernhardoj commented 6 months ago

I haven't find a fix for it.

isabelastisser commented 6 months ago

@hoangzinh, what's next here? Thanks!

hoangzinh commented 6 months ago

I think we can set the initial price for this issue, and then wait for other proposals.

mvtglobally commented 6 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 6 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.

melvin-bot[bot] commented 6 months ago

@hoangzinh @isabelastisser this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

isabelastisser commented 6 months ago

still waiting for proposals.

isabelastisser commented 5 months ago

still waiting for proposals.

isabelastisser commented 5 months ago

not overdue.

isabelastisser commented 5 months ago

@mvtglobally I can't reproduce this, can you?

isabelastisser commented 5 months ago

Bump @hoangzinh @mvtglobally, can you reproduce this? If not, let's close this.

bernhardoj commented 5 months ago

I can still reproduce it. I think this issue could potentially be fixed by https://github.com/Expensify/App/pull/41115. The only thing missing is adding a font file for mono italic and mono bold italic which I suggested in my proposal too.

Then, add a new font asset for ExpensiMono-Bold and ExpensiMono-BoldItalic

Better to hold for it, I guess.

isabelastisser commented 5 months ago
isabelastisser commented 4 months ago

@hoangzinh are we still holding this one?

hoangzinh commented 4 months ago

@hoangzinh are we still holding this one?

I think yes, they're standardizing font usage. @bernhardoj proposal would like to add mono italic and mono bold italic. Therefore I think we should avoid conflicting with them

isabelastisser commented 3 months ago

Not overdue, still on hold.

isabelastisser commented 2 months ago

not overdue

isabelastisser commented 1 month ago

@hungvu193 still on hold?

hoangzinh commented 1 month ago

@isabelastisser Nope, the holding issue has been merged https://github.com/Expensify/App/pull/41115, I think we can remove the hold label and start waiting for proposals

cc @bernhardoj just in case you're still interested in this issue.

isabelastisser commented 1 month ago

We need proposals!!

bernhardoj commented 1 month ago

I've updated my proposal.

isabelastisser commented 1 month ago

@hoangzinh, can you please review the updated proposal? thanks!

hoangzinh commented 4 weeks ago

Ah sorry, I will review today.

hoangzinh commented 4 weeks ago

@bernhardoj do you have any recording to prove it works? I tried to replace https://github.com/Expensify/App/blob/837152e3df81ebecb2e1166f4f2ddf79ef968674/src/styles/index.ts#L206 with FontUtils.fontFamily.platform.EXP_NEUE, but italic is still not applied in Android app

Screenshot 2024-10-04 at 22 54 41

Moreover, can you elaborate on why it only happens in native apps?

bernhardoj commented 4 weeks ago

do you have any recording to prove it works?

I also replaced it with NEUE. From your screenshot, looks like the italic is successfully applied. If you want to use NEUE, you need to change the family here. https://github.com/Expensify/App/blob/99f280b0edae75ff59614a1c5e98c47b39800aa5/src/components/HTMLEngineProvider/HTMLRenderers/CodeRenderer.tsx#L21

I'll remove that and only rely on the family from the styles file.

Surprisingly though we don't need the new font file on Android for the italic and I just learned that Android by default will apply a fake effect (italic/bold) to it.

Moreover, can you elaborate on why it only happens in native apps?

I think this was already answered before in https://github.com/Expensify/App/issues/39623#issuecomment-2053523504 plus the fake effect explained above.

hoangzinh commented 3 weeks ago

@bernhardoj before we go to the next step, can you please confirm if your proposal works with Android mWeb too?

bernhardoj commented 3 weeks ago

It works fine without my proposal, just like web.

android mWeb

image

iOS mWeb

image
hoangzinh commented 3 weeks ago

You're right, but after I tried to remove those lines (as you suggested in your proposal), the italic style is not applied in mWeb (I guess because I havent' installed the font, but I would like to confirm, just in case). https://github.com/Expensify/App/blob/7981265dc14bc9cb99b512baddbb027446568fcf/src/components/HTMLEngineProvider/HTMLRenderers/CodeRenderer.tsx#L35-L36 Screenshot 2024-10-07 at 17 10 20