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

Update design tokens #378

Merged
merged 4 commits into from
Sep 4, 2024
Merged

Update design tokens #378

merged 4 commits into from
Sep 4, 2024

Conversation

tuentisre
Copy link
Collaborator

@tuentisre tuentisre commented Aug 29, 2024

This PR was automatically created by the import-design-tokens GitHub Action.

🎟️ Jira ticket

ANDROID-15048

🥅 What's the goal?

Update new Mistica tokens and give support for mistica-android.

🚧 How do we do it?

  • Launch 'Update design tokens' action in 'mistica-android' project.
  • Replace some renamed tokens: button...Selected to button...Pressed
  • Add tow new Titles (Title3 and Title4)
    • Title1 remains the same as before.
    • Title2 is now built with a new preset values.
    • Title3 replace the old Title2, so all old references to Title2 should be replaced to Title3.
    • Title4 is completely new (like the new Title2), and should be built with preset6 values.
drawing

☑️ Checks

  • I updated the documentation, including readmes and wikis. If this is a breaking change, tag the PR with "Breaking Change" label and remember to include breaking change migration guide in release notes where this version is released.
  • Tested with dark mode.
  • Tested with API 24. The two new components work well in 24 but I had to use talkback on a device with api level 28, I couldn't manage to run talkback on 24-27 devices.
  • Sync done with iOS team for this feature to ensure alignment, if applies.
  • Reviewed by Mistica design team (PR reviewer from design team)

🧪 How can I test this?

Telefonica Movistar O2 O2 New
telefonica movistar o2 o2_new
Vivo Vivo New Blau Tu
vivo vivo_new blau tu

@tuentisre tuentisre requested review from a team, aweell, DevPabloGarcia and juangardi21 and removed request for a team August 29, 2024 07:28
Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

@@ -21,7 +23,8 @@ fun Titles() {
.padding(16.dp)
) {
DefaultTitles()
TitleStyle.values().forEach {
TitleStyle.entries.forEach {
Spacer(modifier = Modifier.height(24.dp))
Copy link
Contributor

@haynlo haynlo Aug 30, 2024

Choose a reason for hiding this comment

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

Just to make Title catalog easier to check for each Title type.

Copy link
Contributor

@jeprubio jeprubio Aug 30, 2024

Choose a reason for hiding this comment

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

We could also make TitlesWithStyleOverriden accept a modifier by parameter and set padding there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but the Composable TitlesWithStyleOverridden does not have a root composable component like Row, Column or Box, so I could not apply this parameterized modifier. I added a Spacer instead to make it as simple as possible with minimal code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Improve id's naming and add new Titles

Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from the change in titles, the names of the buttons that ended in "Selected" have also been changed to "Pressed"

Comment on lines 77 to +80
presetTitle1FontWeight = brand.title1FontWeight,
presetTitle2FontWeight = brand.title2FontWeight,
presetTitle3FontWeight = brand.title3FontWeight,
presetTitle3FontSize = brand.title3FontSize,
Copy link
Contributor

@haynlo haynlo Aug 30, 2024

Choose a reason for hiding this comment

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

  • Title1 -> Remains the same as before
  • Title2 -> based on Preset3 with custom font-weightfor each brand
  • Title3 -> based on Preset5 (as it was configured in the old Title2) with custom font-weight and text-size for each brand
  • Title4 -> based on Preset6 without any customization, so that's why I didn't add a specific preset for Title4.

ℹ️ You will see the same configuration in the XML-side.
More info here: https://www.figma.com/design/yQRVCOmRQNM98bnXndLPxK/%F0%9F%94%B8-Title-Specs?node-id=504-624&t=wuNyXP4uuHApFHcf-4

Comment on lines -48 to +54
get() = super.values.copy(titleStyle = TitleStyle.TITLE_2)
get() = super.values.copy(titleStyle = TitleStyle.TITLE_3)
Copy link
Contributor

@haynlo haynlo Aug 30, 2024

Choose a reason for hiding this comment

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

Before all these changes, Movistar was the only Brand with Title2 style by default (the other brands had and will continue to have the Title1 style by default).

With the new Title changes, all the Title2 references should be replaced by the new Title3 style, so that's why I've changed this value here.

ℹ️ You will see the same configuration in the XML-side.
More info here: Telefonica/mistica-design#1796 (comment)

🍏 iOS alignment: Telefonica/mistica-ios#397 (comment)

textSize: TextUnit = 20.sp,
) = buildPreset5().copy(
fontWeight = fontWeight,
fontSize = textSize,
)
Copy link
Contributor

@haynlo haynlo Aug 30, 2024

Choose a reason for hiding this comment

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

Remember: I am not building a PresetTitle4 since this title is using Preset6 without any customization.
Usage: https://github.com/Telefonica/mistica-android/pull/378/files#diff-92dbc5d50904a858fa74ff3a03ba997ab7a1c8ac4a8ebc43927c64b633b46810R99

Copy link
Contributor

@haynlo haynlo Aug 30, 2024

Choose a reason for hiding this comment

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

@haynlo haynlo requested review from a team, jeprubio and haynlo and removed request for a team and haynlo August 30, 2024 11:33
@haynlo haynlo requested review from pmartinbTEF, a team and andredelramo-hm and removed request for a team August 30, 2024 11:33
@nimeacuerdo nimeacuerdo requested a review from dpastor August 30, 2024 12:16
Copy link
Contributor

@jeprubio jeprubio left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -21,7 +23,8 @@ fun Titles() {
.padding(16.dp)
) {
DefaultTitles()
TitleStyle.values().forEach {
TitleStyle.entries.forEach {
Spacer(modifier = Modifier.height(24.dp))
Copy link
Contributor

@jeprubio jeprubio Aug 30, 2024

Choose a reason for hiding this comment

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

We could also make TitlesWithStyleOverriden accept a modifier by parameter and set padding there.

Copy link
Contributor

@pmartinbTEF pmartinbTEF left a comment

Choose a reason for hiding this comment

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

Good job!

Copy link
Contributor

@dpastor dpastor left a comment

Choose a reason for hiding this comment

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

Please, tag it with "Breaking change" and remember to write breaking changes on release notes when published.
Check also we are pointing to same mistica design tag than iOS.
I would also add a ticket to the task, so we have it tracked changes in jira.

@haynlo haynlo added the Breaking Change A change that is not retrocompatible label Sep 2, 2024
@yceballost yceballost requested review from aweell and removed request for andredelramo-hm September 2, 2024 10:02
Copy link
Contributor

@yceballost yceballost left a comment

Choose a reason for hiding this comment

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

🤩

Copy link
Contributor

@haynlo haynlo left a comment

Choose a reason for hiding this comment

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

✏️

@haynlo haynlo merged commit 4650877 into main Sep 4, 2024
6 checks passed
@haynlo haynlo deleted the import-design-tokens branch September 4, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change A change that is not retrocompatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants