gonzalezreal / swift-markdown-ui

Display and customize Markdown text in SwiftUI
MIT License
2.53k stars 318 forks source link

Recent change to dynamic scaling broke themes that use custom fonts #337

Closed mustiikhalil closed 2 months ago

mustiikhalil commented 3 months ago

Hello,

Thanks for maintaining this library. We are facing an issue with the dynamic text scaling and custom fonts whenever we try to use the library after the latest update 2.3.1. In the latest release there is this following PR that adjusts the use of "setting the dynamic text twice". As seen below in the images the when using the markdownTheme in the green box it doesnt change the size of the text while the user is changing the text size.

The project:

We use the following SwiftUI view as an example:

struct ContentView: View {
    let text = """
    ### Hello, world!

    This is text
    """
    var body: some View {
        VStack(spacing: 10) {
            Text(text) // This should only display the text without any changes for the sake of sanity checks
                .font(.custom("menlo", size: size))
                .background(Color.red)

            Markdown(MarkdownContent(text)) // This renders the markdown properly
                .markdownTextStyle {
                    FontFamily(.custom("menlo"))
                    FontSize(size)
                }
                .background(Color.blue)

            Markdown(MarkdownContent(text))  // This fails to render the markdown properly
                .markdownTheme(.markdownTheme)
                .background(Color.green)
        }
        .padding()
    }
}

Theme:

extension Theme {
    static let markdownTheme = Theme()
        .heading3 { config in
            config.label
                .markdownTextStyle {
                    FontFamily(.custom("menlo"))
                    FontSize(28)
                }
        }
        .paragraph { config in
            config.label
                .markdownTextStyle {
                    FontFamily(.custom("menlo"))
                    FontSize(size)
                }
        }
        .text {
            FontFamily(.custom("menlo"))
            FontSize(size)
        }
}

Screenshots: Default Sized text:

simulator_screenshot_880F589A-6DDA-42F6-A5BE-5E4B2154C9CB

When user sets text bigger:

simulator_screenshot_95C3E170-1248-4766-BECC-680DC86EF05C

Screenshots from 2.3.0:

Default Sized text:

simulator_screenshot_0AC0F883-3134-42A4-A015-04A64CD243D6

When user sets text bigger:

simulator_screenshot_EA1770B0-69E9-4CEC-8304-81393A389889

mustiikhalil commented 3 months ago

cc: @gonzalezreal

gonzalezreal commented 3 months ago

Sorry, I am currently unavailable. I can look into this in a couple of weeks.

gemigkaka commented 2 months ago

@gonzalezreal Hi! Have you got any time to look into this yet? I could open a PR to fix it, but since that would just be a revert of this PR, I'm not sure how to proceed.

gonzalezreal commented 2 months ago

Hi @gemigkaka, I still haven't got the time to look into this. I need to review #314 again before reverting that PR. I will take a look as soon as I can.

gonzalezreal commented 2 months ago

Hi @mustiikhalil & @gemigkaka,

Sorry for taking so long to look into this. Thanks for your patience :)

The code introduced in #314 is correct, as previously the dynamic type scale was applied twice. The issue with the code you have provided in the description is that you are using an absolute point size for the font in the paragraph and heading3 styles, and this is causing the font scale to reset to 1.

https://github.com/gonzalezreal/swift-markdown-ui/blob/55441810c0f678c78ed7e2ebd46dde89228e02fc/Sources/MarkdownUI/Theme/TextStyle/Styles/FontSize.swift#L24-L37

Before #314 this was working for your use case because the scale was being incorrectly applied twice.

If you remove the custom paragraph style from the custom theme in the description, you will see that the paragraph text scales properly while the heading text doesn't, because the FontSize(28) is resetting the scale to 1.

The recommended way of adjusting the font size for the different block and text styles is to use a relative size, either based on the parent font size (.em) or the root font size (.rem). Use an absolute point size only for the base text style.

The following theme scales the text properly when changing the dynamic type:

extension Theme {
  static let myTheme = Theme()
  .heading3 { config in
    config.label
    .markdownTextStyle {
      FontFamily(.custom("trebuchet MS"))
      FontWeight(.semibold)
      FontSize(.em(1.5))
    }
  }
  .text {
    FontFamily(.custom("menlo"))
    FontSize(16)
  }
}

I am closing this issue as the library is behaving as expected.