search

chrisdhaan / CDMarkdownKit

An extensive Swift framework providing simple and customizable markdown parsing.
MIT License
254 stars 66 forks source link

Improved font size conversion in header element #12

Closed FelixLisczyk closed 6 years ago

FelixLisczyk commented 6 years ago

Issue Link :link:

I've noticed that UIKit changes the family of certain fonts dynamically based on the font size. Here is an example of this behavior:

let font = UIFont.systemFont(ofSize: 17)    // font-family: ".SFUIText"
let biggerFont = font.withSize(20)          // font-family: ".SFUIDisplay"

I've modified the header element to reflect these changes.

Testing Details :mag:

No tests were added.

chrisdhaan commented 6 years ago

I'm not quite sure I understand. You've modified the header element to reflect changing the fonts dynamically based on size?

FelixLisczyk commented 6 years ago

I'll try to provide a better example:

Create a UITextView and set its font to System 17.0. Launch the app and print out the text view's font. The output should look like this:

<UICTFont: 0x7feb8674dec0> font-family: ".SFUIText"; font-weight: normal; font-style: normal; font-size: 17.00pt

Now change the font to System 20.0 and launch the app again. The output changes to this:

<UICTFont: 0x7fe524e23ca0> font-family: ".SFUIDisplay"; font-weight: normal; font-style: normal; font-size: 20.00pt

As you can see that did not only change the font size, but also the font family from "San Francisco Text" to "San Francisco Display". In my tests the latter font family is used for all font sizes >= 20.

Currently, CDMarkdownHeader changes the font size by creating a new font object with the same name and a new size. This initializer always returns a font of the same family and doesn't respect UIKit's dynamic font behavior. Therefore I let UIKit convert the font with its internal logic instead of creating a new font manually. I also wrote an extension for NSFont, which can convert fonts using NSFontManager.

FelixLisczyk commented 6 years ago

I've moved the NSFont extension into UIFont+CDMarkdownKit.swift to stay coherent with the implementation in #13. After merging both PRs there may be a duplicate declaration of NSFontManager, which has to be removed.

chrisdhaan commented 6 years ago

Thanks for the explanation. I understand the problem much better now. I see that you chose to only update this for the Header element. Is there a reason you chose to focus on that specific element and not other text based elements?

FelixLisczyk commented 6 years ago

The CDMarkdownHeaderis the only element that changes its font size after initialization. Did you find any other elements where this issue occurs?

chrisdhaan commented 6 years ago

Good point. I'll get this change pushed up.

chrisdhaan commented 6 years ago

@FelixII I'm about to push up these changes. To be clear, this is only a macOS issue correct?

FelixLisczyk commented 6 years ago

This was primarily an iOS issue. I merely wrote the macOS extension because NSFont doesn't have its own withSize(CGFloat)method (unlike UIFont).

I've just run a few more tests on macOS and noticed that NSFontManager doesn't change the font family either. I haven't found out yet how to replicate the system font behavior on macOS (so that 'San Francisco Text' becomes 'San Francisco Display' at size 20).

Therefore this PR is currently only a fix for iOS and at most a small refactoring for macOS.

chrisdhaan commented 6 years ago

This work has been implemented. Closing out PR.