Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix italic font #549

Merged
merged 13 commits into from
Oct 2, 2020
Merged

Fix italic font #549

merged 13 commits into from
Oct 2, 2020

Conversation

sketchydroide
Copy link
Contributor

@sketchydroide sketchydroide commented Sep 28, 2020

fixes: https://github.com/Expensify/Expensify/issues/141857

Problem

Italic fonts needed to be added.

Tests

  1. Write a comment in italic one that has a g and comment in normal style, also with a g
  2. Verify that the italic style is shown on all platforms, and it is the same font as the normal comment. The word with a g helps because this letter is very specific to this font.

@sketchydroide sketchydroide self-assigned this Sep 28, 2020
@sketchydroide sketchydroide marked this pull request as draft September 28, 2020 14:34
package.json Outdated
@@ -20,7 +20,8 @@
"lint": "eslint .",
"postinstall": "patch-package",
"postversion": "react-native-version",
"print-version": "echo $npm_package_version"
"print-version": "echo $npm_package_version",
"link": "react-native link"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a command to link the resources and in particular the fonts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's my understanding that react native should link things for us: e.g. https://github.com/react-native-community/cli/blob/master/docs/autolinking.md

I have run link before because I was reading old documentation for libraries and it lead to issues compiling iOS/Android. My recommendation would be remove this so people don't use it and break the build, and just use it manually for linking fonts. Maybe you could go one step further and write a stack overflow post on "how to add a custom font to react native"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed and I added the SO here

@sketchydroide sketchydroide requested a review from a team October 1, 2020 15:30
@sketchydroide sketchydroide marked this pull request as ready for review October 1, 2020 15:30
@botify botify requested review from alex-mechler and removed request for a team October 1, 2020 15:30
tgolen
tgolen previously requested changes Oct 1, 2020
// if the style is italic, and iOS won't use italic font if the style is normal, web does care either way.
// this will probably need some change on the HTML library itself, so for now I'm adding
// the platform check
fontStyle: Platform.OS === 'android' ? 'normal' : 'italic',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding a platform check (which is forbidden), the proper way is to use the platform specific file names. In this case, I would do something like this:

  • Add the directory /src/style/italic/
  • Add two files to it index.js (which exports "italic") and index.android.js (which exports "normal")
  • In this file import italic from './italic';
  • and change this reference to fontStyle: italic,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds like a excellent solution, I might need something similar for a fix on for bold not working on Android

@AndrewGable
Copy link
Contributor

Looking super close! Just one lint error to fix.

removing the link command;
@AndrewGable AndrewGable dismissed tgolen’s stale review October 2, 2020 18:22

Changes were made to resolve this 👍

@AndrewGable AndrewGable merged commit d5e9e25 into master Oct 2, 2020
@AndrewGable AndrewGable deleted the afonseca_fix_italic_font branch October 2, 2020 18:22
kidroca added a commit to kidroca/Expensify.cash that referenced this pull request Feb 19, 2022
Regarding the breaking change - we're not using SSID information - it doesn't affect us

v8 Includes the following fixes:
8.0.0 (2022-02-10)

BREAKING CHANGES
it's possible this is a breaking change, depending on your app use case. If you relied on iOS SSID information and met Apple's strict criteria for accessing SSID, you need to set the new config value shouldFetchWiFiSSID to true. If you set it to true and do not meet the criteria your app may crash due to a memory leak. All versions prior to 7.1.12 would attempt to fetch the information regardless of permission, leak memory, and possibly crash. This change avoids that crash.

Bug Fixes
ios: avoid memory leak from ssid APIs by adding explicit config (Expensify#560) (fbf7c15), closes Expensify#420

7.1.11 (2022-02-08)
Bug Fixes
windows: fix race condition in WiFi connection details feature (Expensify#568) (0cd8132)

7.1.10 (2022-02-07)
Bug Fixes
android: use registerDefaultNetworkCallback so toggling airplane mode works (Expensify#571) (e8af2de)

7.1.9 (2022-01-26)
Bug Fixes
android: count native listeners / correctly disable listener if count == 0 (Expensify#569) (5ae16f6)

7.1.8 (2022-01-25)
Bug Fixes
windows: refactor implementation to avoid crashes (Expensify#564) (cc4bfa3)

7.1.7 (2021-12-20)
Bug Fixes
android: populate network value during initial module startup (Expensify#553) (c05080f)

7.1.6 (2021-12-13)
Bug Fixes
android: avoid send event when has no listener (Expensify#548) (cad47d8)

7.1.5 (2021-12-09)
Bug Fixes
android: use method-local ref to instance var for multi-thread safety Expensify#549 (Expensify#550) (81bbc87)

7.1.4 (2021-12-07)
Bug Fixes
android: try async state fetch as stale state workaround (Expensify#547) (937cf48), closes Expensify#542
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants