-
Notifications
You must be signed in to change notification settings - Fork 6
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-10491 Update Title component #397
Conversation
|
||
var font: UIFont { | ||
switch self { | ||
case .title1: | ||
return .textPreset1(weight: .title1) | ||
case .title2: | ||
return .textPreset5() | ||
return .textPreset3(weight: .title2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we migrate this @aweell ??? Checking this, where we used title2
, now we have to use... title3
right?
case title3 | ||
case title4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new ones!
@@ -40,7 +40,7 @@ public extension FontStyle { | |||
} | |||
|
|||
enum TextPreset3Weight: CaseIterable { | |||
case light, regular, medium, button, link | |||
case light, regular, medium, button, link, title2, title3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a little bit weird. textPreset3
can have title2
weight 🤷🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're messing and reusing fonts and weights namings, we should use bold, heavy, light, medium, regular, semibold, thin, ultralight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's due to bold
is bold
, light
is light
but title2
is a custom weight which depends on the brand.
For example:
case .textPreset5, | ||
.textTitle3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered textTitle3
as a UIKit
.title3
(checking the sizes)
@@ -168,7 +174,8 @@ private extension FontStyle { | |||
return .body | |||
case .textPreset4: | |||
return .headline | |||
case .textPreset5: | |||
case .textPreset5, | |||
.textTitle3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered textTitle3
as a UIKit
.title3
(checking the sizes)
@@ -73,6 +73,78 @@ final class TitleHeaderFooterViewTests: XCTestCase { | |||
viewBuilder: makeSectionTitle(title: "This is a very long test text to check multiline", linkTitle: "Link text", style: .title2) | |||
) | |||
} | |||
|
|||
func testTitle3() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title 3 uses different presets depending on the brand so recording this for two brands is enough IMO
) | ||
} | ||
|
||
func testTitle4() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title 4 uses the same text preset for any brand so recording this for one brand is enough IMO
@@ -55,9 +55,51 @@ func assertSnapshotForAllBrandsAndStyles<View: UserInterfaceStyling, Format>( | |||
} | |||
} | |||
|
|||
func assertSnapshot<View: UserInterfaceStyling, Format>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useful to select in which brand you want to record screenshots. It will save us a lot of screenshots, sizes, time,... (talked in our previous platform evolution meeting)
@@ -171,7 +171,7 @@ private extension TitleView.Style { | |||
static var `default`: Self { | |||
switch MisticaConfig.brandStyle { | |||
case .movistar: | |||
return .title2 | |||
return .title3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aweell I found this.
If we don't provide a style, it was taking the default one. Since the title2
becomes the title3
, I've changed it to avoid regressions in the apps (where we weren't specifying a concrete style => less breaking changes)
BREAKING CHANGE: Add some title styles and modify the existing ones. Rename the color tokens from mistica-design. * Update tokens * Update tokens again * Fix breaking change * IOS-10491 Update Title component (#397) * Update Title styles * Fix current screenshots * Add tests for new title styles * Add new title styles to the catalog * Keep the previous one as default to avoid breaking changes * Fix font tests due to new tokens
# [33.0.0](v32.0.0...v33.0.0) (2024-08-26) ### Feat * **Title:** Update title component and Mistica tokens ([#396](#396)) ([d3df994](d3df994)), closes [#397](#397) ### BREAKING CHANGES * **Title:** Add some title styles and modify the existing ones. Rename the color tokens from mistica-design. * Update tokens * Update tokens again * Fix breaking change
Note
This PR starts from #396 which got the new tokens needed here.
They are requested in a different PR since it's not closed yet (the tokens come from a custom branch and they must come from
production
).🎟️ Jira ticket
https://jira.tid.es/browse/IOS-10491
🥅 What's the goal?
Telefonica/mistica-design#1796
🚧 How do we do it?
You can review this separately:
title3
&title4
and modified thetitle2
-> ece02cetitle2
(the modified one) -> bae3c36🧪 How can I verify this?
🏑 AppCenter build
https://install.appcenter.ms/orgs/Tuenti-Organization/apps/Mistica-SwiftUI-iOS/distribution_groups/public/releases/67