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

ANDROID-14825 Add fonts in Mistica Catalog app #394

Merged
merged 5 commits into from
Nov 6, 2024

Conversation

pmartinbTEF
Copy link
Contributor

@pmartinbTEF pmartinbTEF commented Oct 31, 2024

🥅 What's the goal?

Add fonts in Mistica Catalog app

🚧 How do we do it?

Added font archives and created correspondent themes for xml and Brand objects for Compose

☑️ 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.
  • Sync done with iOS team for this feature to ensure alignment, if applies.
  • Accessibility considerations.

🧪 How can I test this?

v14.1.0 With fonts v15.0.1 With fonts
drawing drawing
drawing drawing

Copy link

📱 New catalog for testing generated: Download

ComponentStyle("O2", R.style.CatalogO2, O2),
ComponentStyle("Vivo", R.style.CatalogVivo, VIVO),
ComponentStyle("Vivo New", R.style.CatalogVivoNew, VIVO_NEW),
ComponentStyle("Telefonica", R.style.CatalogTelefonica, TELEFONICA),
ComponentStyle("Blau", R.style.MisticaTheme_Blau, BLAU),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blau has a licensed font

Copy link
Member

Choose a reason for hiding this comment

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

Even if we dont have that font in our catalog could we have a CatalogBlau like the rest of the brands, even if is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link

github-actions bot commented Nov 4, 2024

📱 New catalog for testing generated: Download

@pmartinbTEF pmartinbTEF marked this pull request as ready for review November 4, 2024 12:00
Copy link

github-actions bot commented Nov 4, 2024

📱 New catalog for testing generated: Download

Comment on lines 21 to 22
override val compatibilityTheme: Int
get() = R.style.MisticaTheme_Movistar
Copy link
Contributor

@yamal-alm yamal-alm Nov 4, 2024

Choose a reason for hiding this comment

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

this should be CatalogMovistar so AndroidViews still use the correct font. Same for other brands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

import com.telefonica.mistica.compose.theme.color.MisticaColors
import com.telefonica.mistica.compose.theme.values.MisticaRadius

object CatalogMovistarBrand : Brand {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can create a CatalogBrand and reuse it so we don't have to write all this code for every brand:

abstract class CatalogBrand(
     private val basebBrand: Brand,
     val theme: Int,
) : Brand {
     override val compatibilityTheme: Int = theme
     override val lightColors: MisticaColors = brand.lightColors
     // ...
}

The you can use it as follows:

object CatalogMovistarBrand : CatalogBrand(MovistarBrand, R.style.CatalogMovistar) {
    override val fontFamily: FontFamily
        get() = FontFamily(
            Font(R.font.onair_light, FontWeight.Light),
            Font(R.font.onair_regular, FontWeight.Normal),
            Font(R.font.onair_medium, FontWeight.Medium),
        )
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@pmartinbTEF pmartinbTEF requested a review from yamal-alm November 5, 2024 12:01
Copy link

github-actions bot commented Nov 5, 2024

📱 New catalog for testing generated: Download

)
}

object CatalogO2Brand : CatalogBrand(O2Brand, R.style.CatalogO2) {
Copy link
Member

@dagonco dagonco Nov 5, 2024

Choose a reason for hiding this comment

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

A minor, could we have O2 with his brother O2New together as Vivo is with its two themes?
Also, check XML too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

val theme: Int,
) : Brand {
override val compatibilityTheme: Int
get() = theme
Copy link
Member

Choose a reason for hiding this comment

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

Now seeing this, it is necessary that theme is computed every time that compatibilityTheme is retrieved? cc: @yamal-alm

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, we can use override val compatibilityTheme: Int = theme instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@dagonco dagonco left a comment

Choose a reason for hiding this comment

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

Cool, nice job!

Copy link
Contributor

@yamal-alm yamal-alm 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

github-actions bot commented Nov 6, 2024

📱 New catalog for testing generated: Download

Copy link

@aweell aweell left a comment

Choose a reason for hiding this comment

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

LGTM

@pmartinbTEF pmartinbTEF merged commit 702ce27 into main Nov 6, 2024
5 checks passed
@pmartinbTEF pmartinbTEF deleted the ANDROID-14825-fonts_in_catalog branch November 6, 2024 15:54
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