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

Define type styles in WKFont, update project to use only WKFont #4866

Merged
merged 35 commits into from
Jul 18, 2024

Conversation

mazevedofs
Copy link
Collaborator

@mazevedofs mazevedofs commented Jun 18, 2024

Phabricator: https://phabricator.wikimedia.org/T300034

Notes

  • This PR updates the fonts styles according to the Components guidelines in Figma
  • Updates all fonts used in the project to use WKFont
  • Removes old font extension and WMFDynamicType
  • Creates a wrapper to expose WKFont to ObjC without compile issues
  • Uses new HTML Utils for HTMl parsing and styling

Since we have less styles now, I swapped some fonts based on approximate size and weight

Test Steps

  1. Run the app, do a light smoke test in all screens
  2. Test dynamic type

Heavier smoke testing will be done by the QA

Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

@mazevedofs Great stuff! Some comments after an initial (very quick) code-only review. I will dig deeper in the near future.

WMF Framework/String+HTML.swift Show resolved Hide resolved
Wikipedia/Code/ArticleCollectionViewCell.swift Outdated Show resolved Hide resolved
Wikipedia/Code/EditSaveViewController.swift Show resolved Hide resolved
Wikipedia/Code/NewsCollectionViewCell.swift Outdated Show resolved Hide resolved
Wikipedia/Code/ArticleCollectionViewCell.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

I found a few more comments after a slower code review.

Components/Sources/Components/Style/WKFont.swift Outdated Show resolved Hide resolved
Components/Sources/Components/Style/WKFont.swift Outdated Show resolved Hide resolved
Components/Sources/Components/Style/WKFont.swift Outdated Show resolved Hide resolved
Components/Sources/Components/Style/WKFont.swift Outdated Show resolved Hide resolved
Components/Sources/Components/Style/WKFont.swift Outdated Show resolved Hide resolved
Components/Sources/Components/Style/WKFont.swift Outdated Show resolved Hide resolved
WMF Framework/UITraitCollection+Extensions.swift Outdated Show resolved Hide resolved
@tonisevener
Copy link
Collaborator

@mazevedofs FYI, I also pushed an bug fix of something existing that's been bothering me. For Image Recommendations and Watchlist, our SwiftUI views do not update the font size upon foreground after changing dynamic type in Settings. This commit fixes it.

@tonisevener
Copy link
Collaborator

tonisevener commented Jul 11, 2024

@mazevedofs

I notice the font has changed enough on this widget to cause a lot of additional spacing at the bottom:

IMG_0371

@mazevedofs
Copy link
Collaborator Author

I notice the font has changed enough on this widget to cause a lot of additional spacing at the bottom:

@tonisevener This widget used SwiftUI caption (the same as caption1 on UIKit), so if there's a significant difference between them, we might need to adjust the Widget.
I didn't notice in my test, but I used a smaller device (a 13 mini).
In the screenshot, the light mode version is the App Store app and the dark mode version uses the new WKFont
Screenshot 2024-07-17 at 11 07 27

@tonisevener
Copy link
Collaborator

I notice the font has changed enough on this widget to cause a lot of additional spacing at the bottom:

@tonisevener This widget used SwiftUI caption (the same as caption1 on UIKit), so if there's a significant difference between them, we might need to adjust the Widget. I didn't notice in my test, but I used a smaller device (a 13 mini). In the screenshot, the light mode version is the App Store app and the dark mode version uses the new WKFont Screenshot 2024-07-17 at 11 07 27

Hmm yeah that is strange, I'm not sure why the difference is occurring. What's interesting is that all font sizes seem to have shrunk for me. I'm testing on my iPhone 12 Pro Max.

main:
IMG_0391 2

PR branch:
IMG_0392 2

@tonisevener
Copy link
Collaborator

Per discussion: We'll adjust widgets after another round of design review as-needed.

@tonisevener tonisevener merged commit 98ed102 into main Jul 18, 2024
4 checks passed
@tonisevener tonisevener deleted the components-fonts-update branch July 18, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants