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-10491 Update Title component #397

Merged
merged 5 commits into from
Aug 23, 2024
Merged

Conversation

amegias
Copy link
Contributor

@amegias amegias commented Aug 21, 2024

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?

Create new title between 2 and 3. This cause a breaking change in dev side.

Telefonica/mistica-design#1796

Captura de pantalla 2024-08-21 a las 15 38 41

🚧 How do we do it?

You can review this separately:

  1. I've added the title3 & title4 and modified the title2 -> ece02ce
  2. Then, I've fixed the screenshots related with the title2 (the modified one) -> bae3c36
  3. After that, I've added tests for the new titles (3 & 4) -> a5e3dd0
  4. I've added the new ones to the catalog -> 698ee7c

🧪 How can I verify this?

  • See the recorded tests
  • Execute the catalog

🏑 AppCenter build

https://install.appcenter.ms/orgs/Tuenti-Organization/apps/Mistica-SwiftUI-iOS/distribution_groups/public/releases/67

@amegias amegias changed the title Update Title styles IOS-10491 Update Title component Aug 21, 2024

var font: UIFont {
switch self {
case .title1:
return .textPreset1(weight: .title1)
case .title2:
return .textPreset5()
return .textPreset3(weight: .title2)
Copy link
Contributor Author

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?
Captura de pantalla 2024-08-21 a las 15 38 41

Comment on lines +22 to +23
case title3
case title4
Copy link
Contributor Author

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
Copy link
Contributor Author

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 🤷🏻

Copy link
Contributor

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

Copy link
Contributor Author

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:

Comment on lines +152 to +153
case .textPreset5,
.textTitle3:
Copy link
Contributor Author

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:
Copy link
Contributor Author

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() {
Copy link
Contributor Author

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() {
Copy link
Contributor Author

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>(
Copy link
Contributor Author

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)

@amegias amegias marked this pull request as ready for review August 21, 2024 13:59
@@ -171,7 +171,7 @@ private extension TitleView.Style {
static var `default`: Self {
switch MisticaConfig.brandStyle {
case .movistar:
return .title2
return .title3
Copy link
Contributor Author

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)

@amegias amegias mentioned this pull request Aug 22, 2024
@amegias amegias merged commit a411397 into update-titles Aug 23, 2024
1 check passed
@amegias amegias deleted the update-titles-part-2 branch August 23, 2024 12:37
amegias added a commit that referenced this pull request Aug 26, 2024
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
tuentisre pushed a commit that referenced this pull request Aug 26, 2024
# [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
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.

4 participants