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] ♻️ Use the defined Typography on each screen. #1178

Merged
merged 24 commits into from
Sep 12, 2023

Conversation

Corvus400
Copy link
Contributor

@Corvus400 Corvus400 commented Sep 11, 2023

Issue

Overview (Required)

  • The previously defined Typography style is now set up like Android.

Movie (Optional)

Before After

@Corvus400 Corvus400 changed the title [WIP] ♻️ Use the defined Typography on each screen. ♻️ Use the defined Typography on each screen. Sep 11, 2023
@Corvus400 Corvus400 marked this pull request as ready for review September 11, 2023 14:47
@Corvus400 Corvus400 requested a review from a team as a code owner September 11, 2023 14:47
Comment on lines 1 to 17
import SwiftUI

public extension Text {
func textStyle(_ style: TextStyle) -> some View {
return self.font(style.font)
.lineSpacing(style.lineHeight)
.modifier(LetterSpacingModifier(spacing: style.letterSpacing ?? 0))
}
}

public struct LetterSpacingModifier: ViewModifier {
var spacing: CGFloat
public func body(content: Content) -> some View {
content.padding(.leading, spacing/2)
.padding(.trailing, spacing/2)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An extension function to handle TextStyle defined in Typography has been added this time.

public static let bodyLarge = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 16), lineHeight: 24, letterSpacing: 0.15)
public static let bodyMedium = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 14), lineHeight: 20, letterSpacing: 0.25)
public static let bodySmall = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 12), lineHeight: 16, letterSpacing: 0.4)
public static let displayLarge = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 57), lineHeight: 64 - 57)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of iOS, if the lineHeight value is used as it is, the display will be corrupted, so we have changed to use the value obtained by subtracting the lineHeight value from the Font size.

@Corvus400 Corvus400 changed the title ♻️ Use the defined Typography on each screen. [iOS] ♻️ Use the defined Typography on each screen. Sep 11, 2023
@Corvus400
Copy link
Contributor Author

@ry-itto @tkhs0604
For some reason, no reviewer has been assigned again, so please give us a code review. 🙇

public static let bodyLarge = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 16), lineHeight: 24, letterSpacing: 0.15)
public static let bodyMedium = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 14), lineHeight: 20, letterSpacing: 0.25)
public static let bodySmall = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 12), lineHeight: 16, letterSpacing: 0.4)
public static let displayLarge = TextStyle(font: SwiftUI.Font.custom("Montserrat-Regular", size: 57), lineHeight: 64 - 57)
Copy link
Member

@ry-itto ry-itto Sep 11, 2023

Choose a reason for hiding this comment

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

Sorry, I missed my review comment 🙏🏼
Can you apply this for all vairables?

#1135 (comment)

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 am wrong in my perception. 🙇
I only commented on one area, but I have already corrected all the variables this time.

Copy link
Member

@ry-itto ry-itto Sep 12, 2023

Choose a reason for hiding this comment

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

@Corvus400
My review comment is bad.. (That review comment has two meanings..)
What I want to say is "using static access to font name might be better way". 🙏🏼

Like this
FontAssets.Montserrat.regular.font(size: 57)

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
Is this correction correct?

Comment on lines 14 to 15
content.padding(.leading, spacing/2)
.padding(.trailing, spacing/2)
Copy link
Member

Choose a reason for hiding this comment

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

.horizontal might be a better way. 👀 And maybe this modifier not needed if this line uses .horizontal.

Suggested change
content.padding(.leading, spacing/2)
.padding(.trailing, spacing/2)
content.padding(.horizontal, spacing / 2)

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.

LGTM! Thanks! 👍🏼

@ry-itto ry-itto merged commit 39009ab into DroidKaigi:main Sep 12, 2023
4 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] Use the defined Typography on each screen.
2 participants