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

fix(core): use correct fonts for staking confirmation on Delizia #4677

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

romanz
Copy link
Contributor

@romanz romanz commented Feb 26, 2025

Before:

After:

Following #4672.

@romanz romanz added the core Trezor Core firmware. Runs on Trezor Model T and T2B1. label Feb 26, 2025
@romanz romanz self-assigned this Feb 26, 2025
Copy link

github-actions bot commented Feb 26, 2025

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@romanz romanz force-pushed the romanz/description-font branch from 959bc6e to 536e7b9 Compare February 26, 2025 16:34
@romanz romanz added the translations Put this label on a PR to run tests in all languages label Feb 26, 2025
@romanz romanz force-pushed the romanz/description-font branch from 536e7b9 to d6ca581 Compare February 26, 2025 16:54
@romanz romanz changed the title fix(core): use specified description font in ConfirmValue @romanz fix(core): use correct fonts for staking confirmation on Delizia Feb 26, 2025
@romanz romanz changed the title @romanz fix(core): use correct fonts for staking confirmation on Delizia fix(core): use correct fonts for staking confirmation on Delizia Feb 26, 2025
@romanz romanz requested a review from bieleluk February 26, 2025 17:10
@romanz romanz marked this pull request as ready for review February 26, 2025 17:10
@romanz romanz removed the request for review from prusnak February 26, 2025 17:11
@romanz romanz added the altcoin Any non-Bitcoin coins label Mar 3, 2025
@romanz
Copy link
Contributor Author

romanz commented Mar 4, 2025

Rebasing to update UI fixtures:

1:  d6ca58125 = 1:  13c25ecd1 fix(core): use correct fonts for staking confirmation on Delizia
-:  --------- > 2:  644f47407 fixup! fix(core): use correct fonts for staking confirmation on Delizia

@romanz romanz force-pushed the romanz/description-font branch from d6ca581 to 644f474 Compare March 4, 2025 08:17
Copy link
Contributor

@bieleluk bieleluk left a comment

Choose a reason for hiding this comment

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

The main task of fonts differentiation looks good. I have one minor objection regarding the font color.

@@ -527,7 +528,10 @@ impl FirmwareUI for UIDelizia {
) -> Result<impl LayoutMaybeTrace, Error> {
let confirm_main =
ConfirmValue::new(title.unwrap_or(TString::empty()), message, description)
.with_description_font(&theme::TEXT_MAIN_GREY_LIGHT)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit hesitant about this change.
It is correct according to Figma, but it should have the same color as the address which also has wrong color (GREY_EXTRA_LIGHT instead of GREY_LIGHT).
In the test diff screen, there are now 3 different font colors, which does not look good.

I would either change color of both description and address to GREY_LIGHT or none of them.
image

Copy link
Contributor Author

@romanz romanz Mar 4, 2025

Choose a reason for hiding this comment

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

change color of both description and address to GREY_LIGHT

Sounds good, please take a look at #4733.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in #4733, rebasing to resolve UI fixtures' conflicts.

@romanz romanz force-pushed the romanz/description-font branch from 5dd8418 to 8afb0d9 Compare March 5, 2025 09:52
@romanz romanz merged commit f67a506 into main Mar 5, 2025
139 of 140 checks passed
@romanz romanz deleted the romanz/description-font branch March 5, 2025 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
altcoin Any non-Bitcoin coins core Trezor Core firmware. Runs on Trezor Model T and T2B1. translations Put this label on a PR to run tests in all languages
Projects
Status: 🤝 Needs QA
Development

Successfully merging this pull request may close these issues.

2 participants