Define type styles in WKFont, update project to use only WKFont #4866
Conversation
…f the UIFont extension
…nstead of the UIFont extension
…nstead of the UIFont extension, adds some cleanup comments for future commits
…o components-fonts-update
…o components-fonts-update
There was a problem hiding this comment.
@mazevedofs Great stuff! Some comments after an initial (very quick) code-only review. I will dig deeper in the near future.
| // Style Strong | ||
| if let strongColor = styles.strongColor { | ||
| for strongNSRange in allStyleData.strong.completeNSRanges { | ||
| if let strongRange = Range(strongNSRange, in: attributedString) { | ||
| attributedString[strongRange].foregroundColor = strongColor | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This will only add styling to the newer AttributedString utils method, you should also add it to the addStyling(to nsAttributedString: NSMutableAttributedString, allStyleData: AllStyleData, styles: Styles) method in this file.
| @@ -1,30 +0,0 @@ | |||
| import Foundation | |||
|
|
|||
| extension String { | |||
There was a problem hiding this comment.
I'm so happy these old extensions are going away 🙌.
| ] | ||
| ]) | ||
|
|
||
| let styles = HtmlUtils.Styles(font: WKFont.for(.subheadline, compatibleWith: traitCollection), boldFont: WKFont.for(.boldSubheadline, compatibleWith: traitCollection), italicsFont: WKFont.for(.italicSubheadline, compatibleWith: traitCollection), boldItalicsFont: WKFont.for(.boldItalicSubheadline, compatibleWith: traitCollection), color: theme.colors.primaryText, linkColor: theme.colors.link, strongColor: messageEmphasisColor, lineSpacing: 1) |
There was a problem hiding this comment.
I remember the not implementing the additionalTagAttributes in the newer utils, since I wasn't sure how widely used it was. I'm good with leaving this as-is and seeing how design feels about it.
| @@ -14,16 +14,24 @@ open class ArticleCollectionViewCell: CollectionViewCell, SwipeableCell, BatchEd | |||
|
|
|||
| private var _titleHTML: String? = nil | |||
| private var _titleBoldedString: String? = nil | |||
|
|
|||
|
|
|||
| private var theme: Theme = Theme.standard | |||
There was a problem hiding this comment.
I haven't tested yet, so this may not actually be a bug, but since Theme.standard is basically the light theme, and styles are assigned upon setup, I don't see how the styling of the attributed strings will update once the user's theme changes. You may want to play around with automatic theme changing (when Theme is set to default in app settings, then toggle device dark mode) and see how that acts.
| @@ -129,15 +131,18 @@ class EditSaveViewController: WMFScrollViewController, Themeable, UITextFieldDel | |||
|
|
|||
| let substitutedString = String.localizedStringWithFormat( | |||
| localizedString, | |||
| "<a href=\"#LOGIN_HREF\">", // "#LOGIN_HREF" ensures 'byAttributingHTML' doesn't strip the anchor. The entire text view uses a tap recognizer so the string itself is unimportant. | |||
| "<a href=\"#LOGIN_HREF\">", // "#LOGIN_HREF" ensures 'nsAttributedStringFromHtml' doesn't strip the anchor. The entire text view uses a tap recognizer so the string itself is unimportant. | |||
There was a problem hiding this comment.
(I also didn't test this, could be fine) Be sure to confirm whatever this $LOGIN_HREF is doing here still works.
|
|
||
| private var styles: HtmlUtils.Styles { | ||
| HtmlUtils.Styles(font: WKFont.for(.callout, compatibleWith: traitCollection), boldFont: WKFont.for(.boldCallout, compatibleWith: traitCollection), italicsFont: WKFont.for(.italicCallout, compatibleWith: traitCollection), boldItalicsFont: WKFont.for(.boldItalicCallout, compatibleWith: traitCollection), color: theme.colors.primaryText, linkColor: theme.colors.primaryText, lineSpacing: 3) | ||
| // Marina todo - fix link styling |
| if let boldString = _titleBoldedString, let boldFont { | ||
| let boldAttributedString = NSMutableAttributedString(string: boldString) | ||
| let range = NSRange(location: 0, length: boldAttributedString.length) | ||
| boldAttributedString.addAttribute(.font, value: boldFont, range: range) |
There was a problem hiding this comment.
I think you might be losing this work. I think the original code added additional bolding to _titleBoldedString within attributedTitle. Here you are creating a boldAttributedString but not doing anything with it.
Phabricator: https://phabricator.wikimedia.org/T300034
Notes
Since we have less styles now, I swapped some fonts based on approximate size and weight
Test Steps
Heavier smoke testing will be done by the QA