Font-family difference between platforms by 0xthierry · Pull Request #2648 · Expensify/App

0xthierry · 2021-04-30T03:55:49Z

Details

This PR fixes the font-family difference between platform

Fixed Issues

Tests

Send the following message on mobile

  *`test`*
  _`test`_

Italics and bold must be applied.
View the message on desktop / web / ios / android

QA Steps

Click on the text input
Send a message the following messages

1. *`bold`*
2. _`italic`_
3. `normal`
4. *_`italic and bold`_*

The fonts should be applied like in web / desktop

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

shawnborton · 2021-04-30T15:51:10Z

I think we'll also need these fonts defined in CSS for web/desktop/mobileWeb too right? https://github.com/Expensify/Expensify.cash/blob/main/assets/css/fonts.css

Which means that we (on the Expensify side) need to get these fonts uploaded so we can serve them up.

0xthierry · 2021-04-30T16:18:10Z

Which means that we (on the Expensify side) need to get these fonts uploaded so we can serve them up.

I'm adding this in this css file, but yeah, we need to upload this font files to be accessible via https://www.expensify.com/font/ @shawnborton

shawnborton · 2021-04-30T17:36:20Z

Sounds good, I just sent a pull request on our side for these fonts, they should be available early next week.

0xthierry · 2021-04-30T18:08:02Z

nice, thanks @shawnborton 🎉

0xthierry · 2021-05-03T22:51:14Z

Do you could send the name of the fonts? @shawnborton

I saw that the names is different from the file name, like this one:

https://www.expensify.com/font/GT-America-Standard-Bold-Italic.woff") format("woff"), url("https://www.expensify.com/font/GT-America-Standard-Bold-Italic.woff2

shawnborton · 2021-05-04T01:17:21Z

Yeah, the file names are confusing but if we ignore the file names for a moment, the font family's name is GT America Exp, and within this family we have many sub-weights and styles. So for that matter, given that we only have one font family for our brand, I wonder if we don't really need to refer to the formal family name but rather we can just refer to the style? For example, we'd have:

Regular (this is the default)
Regular Italic
Bold
Bold Italic
Monospaced Regular
Monospaced Regular Italic
Monospaced Bold
Monospaced Bold Italic

There should be a corresponding font file for each one of those styles above. Let me know if that makes sense!

0xthierry · 2021-05-04T14:05:20Z

There should be a corresponding font file for each one of those styles above. Let me know if that makes sense!

More or less @shawnborton, because it could be a little confused, in the web we use just refer to the style and in the other platforms (android and ios) we use GTAmericaExpMono-Bd.

In my opinion a good approach is to use the same name in all platforms (web, android and ios)

shawnborton · 2021-05-04T15:52:33Z

If you need to standardize on a font fame name, I think GTAmericaExp works well. Then from here, you would have my list above as the sub-styles of the family.

Luke9389 · 2021-05-05T18:15:21Z

@Thierrysantos To verify, have you tested this on all 5 platforms with the most recent fonts that @shawnborton provided?

0xthierry · 2021-05-06T00:13:01Z

@Luke9389 this changes that @shawnborton made just impact in the web.

Fetching the new fonts from expensify:

Web

Luke9389 · 2021-05-06T00:46:14Z

Awesome. Thanks!

Luke9389 · 2021-05-06T00:46:55Z

All you @TomatoToaster!

parasharrajat · 2021-05-06T04:08:37Z

            TDefaultRenderer={TDefaultRenderer}
            boxModelStyle={boxModelStyle}
-            textStyle={textStyle}
+            textStyle={{...textStyle, ...textStyleOverride}}


We should pass the new styles as Array as per the new StyleGuide. Change the prop type if necessary. Thanks.

I take a look and what this component expect is an object 🤔

Well I am not sure about the style prop that Internal TDefaultRenderer expect so its fine here. Otherwise you can just change that to array type and see how it works.

parasharrajat · 2021-05-06T04:12:24Z

    // "boxModelStyle" corresponds to border, margin, padding and backgroundColor
    const {boxModelStyle, otherStyle: textStyle} = splitBoxModelStyle(style);
+
+    const italic = textStyle.fontStyle === 'italic' && fontFamily.MONOSPACE_ITALIC;


I think this logic should be moved to style.js. for Eg
https://github.com/Expensify/Expensify.cash/blob/53acb9ce968a9ea3007302e3debb8b2d9a345ebb/src/styles/styles.js#L1545-L1550

We can move to styles.js, but I think that is not necessary, because this logic is specific for <CodeRenderer />, so in my option is better keep this logic here.

We like to keep all the style logic out of JSX.

@parasharrajat nice, so I will move to styles.js, thank you 🤘🏽

parasharrajat · 2021-05-06T04:13:47Z

+        fontWeight: undefined,
+        fontStyle: undefined,


Can't we just set it on the default styles for Code blocks as we are just resetting them unconditionally? Also, a comment saying why undefined is used would be helpful as I worked over this a little back and these type of changes does not really make sense until specified.
https://github.com/Expensify/Expensify.cash/blob/53acb9ce968a9ea3007302e3debb8b2d9a345ebb/src/styles/styles.js#L1409

Can't we just set it on the default styles for Code blocks as we are just resetting them unconditionally?

No, when the tags (strong, i, b ...) is within a code block, the style from this tags will be merged and passed as text Style, and with that we can know which font must be used.

If we add undefined in these properties(font Weight and font Family) in this tags(strong, i, b...), when these tags is inside a code block, we can't know which font must be used.
https://github.com/Expensify/Expensify.cash/blob/7a7fd7d8fc2836c9c2f2a4457776c6ec06407fdc/src/components/RenderHTML.js#L67

Also, a comment saying why undefined is used would be helpful as I worked over this a little back and these type of changes does not really make sense until specified.

I will do this change ❤️

parasharrajat · 2021-05-06T04:20:26Z

@Thierrysantos Just posted a few friendly concerns. Thanks.
@Luke9389 Kindly let me know if I am allowed to add review comments. I worked over it earlier so reviewed it to make sure there are no regressions.

0xthierry · 2021-05-06T16:20:07Z

@parasharrajat Thank you for your review 🤘🏽

Luke9389 · 2021-05-06T17:00:47Z

Yea @parasharrajat, feel free to leave review comments!

…Weight and fontStyle

0xthierry · 2021-05-06T22:15:58Z

@Luke9389 @parasharrajat I update the PR with your suggestions 🤘🏽

Luke9389

Thanks for making these changes!

parasharrajat

LGTM

OSBotify · 2021-05-07T18:52:13Z

🚀 Deployed to staging in version: 1.0.39-5🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-05-08T01:43:19Z

🚀 Deployed to production in version: 1.0.39-5🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fix: add the new font-family

c826d47

0xthierry force-pushed the fix/font-family branch from 05c670a to c826d47 Compare April 30, 2021 03:59

0xthierry commented Apr 30, 2021

View reviewed changes

Comment thread src/components/RenderHTML.js

0xthierry marked this pull request as ready for review April 30, 2021 11:49

0xthierry requested a review from a team as a code owner April 30, 2021 11:49

MelvinBot requested review from TomatoToaster and removed request for a team April 30, 2021 11:49

0xthierry mentioned this pull request Apr 30, 2021

Bolded and Italics code does not render properly on mobile #2491

Closed

0xthierry requested a review from Luke9389 May 4, 2021 20:17

Luke9389 previously approved these changes May 5, 2021

View reviewed changes

fix: add the new font-family to web

7e2cb8d

0xthierry dismissed Luke9389’s stale review via 7e2cb8d May 6, 2021 00:09

Luke9389 previously approved these changes May 6, 2021

View reviewed changes

parasharrajat suggested changes May 6, 2021

View reviewed changes

fix: extract the logic to get the font family monospace based in font…

c9c5bcf

…Weight and fontStyle

0xthierry dismissed Luke9389’s stale review via c9c5bcf May 6, 2021 22:12

0xthierry requested a review from Luke9389 May 7, 2021 14:37

Luke9389 approved these changes May 7, 2021

View reviewed changes

parasharrajat approved these changes May 7, 2021

View reviewed changes

Luke9389 merged commit a5c6352 into Expensify:main May 7, 2021

OSBotify mentioned this pull request May 7, 2021

Deploy Checklist: Expensify.cash 2021-04-29 #2639

Closed

23 tasks

bernhardoj mentioned this pull request Apr 5, 2024

[Due for payment 2025-02-18] Android - Chat - Sending code block message with italic is not applied& preview inconsistent #39623

Closed

6 tasks

Conversation

0xthierry commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Uh oh!

shawnborton commented Apr 30, 2021

Uh oh!

0xthierry commented Apr 30, 2021

Uh oh!

shawnborton commented Apr 30, 2021

Uh oh!

0xthierry commented Apr 30, 2021

Uh oh!

0xthierry commented May 3, 2021

Uh oh!

shawnborton commented May 4, 2021

Uh oh!

0xthierry commented May 4, 2021

Uh oh!

shawnborton commented May 4, 2021

Uh oh!

Luke9389 commented May 5, 2021

Uh oh!

0xthierry commented May 6, 2021

Uh oh!

Luke9389 commented May 6, 2021

Uh oh!

Luke9389 commented May 6, 2021

Uh oh!

parasharrajat May 6, 2021

Choose a reason for hiding this comment

Uh oh!

0xthierry May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat May 6, 2021

Choose a reason for hiding this comment

Uh oh!

0xthierry May 6, 2021

Choose a reason for hiding this comment

Uh oh!

parasharrajat May 6, 2021

Choose a reason for hiding this comment

Uh oh!

0xthierry May 6, 2021

Choose a reason for hiding this comment

Uh oh!

parasharrajat May 6, 2021

Choose a reason for hiding this comment

Uh oh!

0xthierry May 6, 2021

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xthierry commented May 6, 2021

Uh oh!

Luke9389 commented May 6, 2021

Uh oh!

0xthierry commented May 6, 2021

Uh oh!

Luke9389 left a comment

Choose a reason for hiding this comment

0xthierry commented Apr 30, 2021 •

edited

Loading

0xthierry May 6, 2021 •

edited

Loading

parasharrajat May 6, 2021 •

edited

Loading

parasharrajat commented May 6, 2021 •

edited

Loading