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

[iOS] ♻️ Enabled to use ttf files placed in commonMain on iOS. #1047

Merged
merged 16 commits into from
Sep 7, 2023

Conversation

Corvus400
Copy link
Contributor

Issue

Overview (Required)

  • Enabled to use ttf files placed in commonMain on iOS.
  • We initially expected to be able to change the fonts of all child views by specifying them in one place, as in Android.
  • However, specifying a custom font in RootView as shown below did not seem to change the appearance of the child views.
  • So we modified all the fonts in the child views.
  • If this is incorrect, please feel free to point this out to us.
import Navigation
import SwiftUI
import Assets

@main
struct MainApp: App {

    init() {
        FontAssets.registerAllCustomFonts()
    }

    var body: some Scene {
        WindowGroup {
            RootView()
                .font(Font.custom(FontAssets.Montserrat.medium.fontName, size: UIFont.systemFontSize))
        }
    }
}
import Navigation
import SwiftUI
import Assets

@main
struct MainApp: App {
    
    init() {
        FontAssets.registerAllCustomFonts()
    }
    
    var body: some Scene {
        WindowGroup {
            RootView()
                .environment(\.font, FontAssets.Montserrat.medium.swiftUIFont(size: UIFont.systemFontSize))
        }
    }
}

Screenshot

Before After
Before After
Before After
Before After
Before After
Before After
Before After
Before After
Before After

@Corvus400 Corvus400 requested a review from a team as a code owner September 2, 2023 07:31
@Corvus400 Corvus400 requested a review from a team as a code owner September 2, 2023 07:33
Comment on lines 17 to 19
// FIXME We initially planned to use this implementation on the iOS side.
//However, when shared.swift is output by XCFramework, methods with Compose annotations are not output.
//Therefore, this implementation cannot be referenced from the iOS side.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Actual method cannot be referenced from the iOS side.
In this case, SwiftGen shares the custom font by referencing the ttf file placed in commonMain.
Please let me know if this is wrong and I will try to fix it.

@github-actions
Copy link

github-actions bot commented Sep 2, 2023

Hi @Corvus400! Codes seem to be unformatted. To resolve this issue, please run ./gradlew detekt --auto-correct and fix the results of ./gradlew lintDebug.. Thank you for your contribution.

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 2, 2023 07:37 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 2, 2023 07:40 Inactive
@github-actions
Copy link

github-actions bot commented Sep 2, 2023

Test Results

211 tests   211 ✔️  7m 32s ⏱️
  11 suites      0 💤
  11 files        0

Results for commit 9fb81f2.

♻️ This comment has been updated with latest results.

@takahirom
Copy link
Member

On the Android side, I think it is good. @ry-itto Do you have any opinion on this?

@ry-itto ry-itto changed the title ♻️ Enabled to use ttf files placed in commonMain on iOS. [iOS] ♻️ Enabled to use ttf files placed in commonMain on iOS. Sep 3, 2023
@Corvus400
Copy link
Contributor Author

@ry-itto
We would appreciate it if you could review this one. 🙇

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 3, 2023 16:30 Inactive
Copy link
Member

@ry-itto ry-itto left a comment

Choose a reason for hiding this comment

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

Almost LGTM! Please check some reviews!

Comment on lines 26 to 33
fonts:
inputs: ../../../../../core/designsystem/src/commonMain/resources/font
outputs:
templateName: swift5
output: Fonts.swift
params:
enumName: FontAssets
publicAccess: true
Copy link
Member

Choose a reason for hiding this comment

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

Can you introduce font with Theme module? 👀
I think font is related to theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ry-itto
Sorry if I have made a recognition error. 🙇
I am aware that this is the content of task #889 , is that correct?
I would like to define typography as a theme in task #889 after introducing it to the iOS project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ry-itto
Please let me confirm. 🙇
Does this mean you want it defined in Theme/swiftgen.yml instead of Assets/swiftgen.yml?

Copy link
Member

Choose a reason for hiding this comment

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

Does this mean you want it defined in Theme/swiftgen.yml instead of Assets/swiftgen.yml?

Yes 🙏🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ry-itto
Thank you very much for your patience. 🙇
Fixed to have Font resource in Theme.

Comment on lines 1 to 3
import Navigation
import SwiftUI
import Assets
Copy link
Member

Choose a reason for hiding this comment

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

Please sort import orders 🙏🏼
I think SwiftLint warns this line when build iOS App.

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 4, 2023 10:59 Inactive
@Corvus400 Corvus400 marked this pull request as draft September 4, 2023 14:40
@Corvus400 Corvus400 changed the title [iOS] ♻️ Enabled to use ttf files placed in commonMain on iOS. [WIP][iOS] ♻️ Enabled to use ttf files placed in commonMain on iOS. Sep 4, 2023
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 5, 2023 10:34 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 5, 2023 11:01 Inactive
@Corvus400 Corvus400 changed the title [WIP][iOS] ♻️ Enabled to use ttf files placed in commonMain on iOS. [iOS] ♻️ Enabled to use ttf files placed in commonMain on iOS. Sep 5, 2023
@Corvus400 Corvus400 marked this pull request as ready for review September 5, 2023 11:35
@Corvus400
Copy link
Contributor Author

@takahirom @ry-itto
Font files are added to the iOS side so that CI builds can pass.
Please review again. 🙇

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 5, 2023 15:34 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 6, 2023 09:22 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 6, 2023 09:58 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 6, 2023 10:28 Inactive
@@ -122,14 +122,17 @@ public struct AboutView<ContributorView: View, StaffView: View, SponsorView: Vie
.padding(.vertical, 24)

Text("アプリバージョン")
.font(Font.system(size: 14, weight: .medium))
.font(Font.custom(FontAssets.Montserrat.medium, size: 14))
.fontWeight(.medium)
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q]
FontAssets.Montserrat.medium seems a medium-sized font, so is this modifier redundant? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkhs0604
I was using the original description as is, so I certainly don't need this modifier.
I have removed it!🫡

@@ -34,16 +34,19 @@ public struct InformationRow: View {
icon
HStack(spacing: 12) {
Text(title)
.font(Font.system(size: 14, weight: .semibold))
.font(Font.custom(FontAssets.Montserrat.medium, size: 14))
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q]
We can embed Montserrat-SemiBold.ttf too, so how about using it instead?

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2023 10:00 Inactive
Copy link
Contributor

@tkhs0604 tkhs0604 left a comment

Choose a reason for hiding this comment

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

Thank you for your great contribution on not only Android but also iOS!
LGTM, and @ry-itto's points have also been resolved, so let me merge it!

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2023 10:36 Inactive
@tkhs0604 tkhs0604 merged commit 802ac66 into DroidKaigi:main Sep 7, 2023
9 checks passed
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.

[iOS] Introduce the new font Montserrat
4 participants