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.

Sign up for GitHub

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

Jump to bottom

refactor(core/ui): optimize brightness settings for Mercury #3991

Merged
merged 2 commits into from
Jul 17, 2024
Merged

refactor(core/ui): optimize brightness settings for Mercury #3991

merged 2 commits into from
Jul 17, 2024

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Jul 2, 2024 •
edited
Loading

Fixes #3969, #3962.

@ibz ibz self-assigned this Jul 2, 2024
@ibz ibz requested a review from prusnak as a code owner July 2, 2024 16:44
@ibz ibz force-pushed the ibz/20240702-brightness branch from 1ce7237 to 3f89663 Compare July 2, 2024 16:46
@ibz ibz requested review from mmilata and removed request for prusnak July 2, 2024 16:46
mmilata
mmilata reviewed Jul 2, 2024
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Some preliminary comments, so far looking good!

If you want to learn more about testing infrastructure feel free to write a device test for this feature.

core/embed/rust/src/ui/model_mercury/layout.rs Outdated Show resolved Hide resolved
core/embed/rust/src/ui/model_mercury/layout.rs Outdated Show resolved Hide resolved
core/embed/rust/src/ui/model_mercury/component/set_brightness.rs Outdated Show resolved Hide resolved
core/embed/rust/src/ui/model_mercury/layout.rs Outdated Show resolved Hide resolved
@Hannsek
Copy link
Contributor

Hannsek commented Jul 3, 2024

@ibz please post here some screenshots from the flow.

@ibz
Copy link
Contributor Author

ibz commented Jul 3, 2024

@ibz please post here some screenshots from the flow.

image
image
image

mmilata
mmilata reviewed Jul 9, 2024
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good, however the appearance is different from figma. Perhaps it changed in the meanwhile? It has orange heading while the finished flows have green so maybe it's still not final?

core/translations/en.json Outdated
@@ -81,7 +81,7 @@
"bitcoin__unverified_external_inputs": "The transaction contains unverified external inputs.",
"bitcoin__valid_signature": "The signature is valid.",
"bitcoin__voting_rights": "Voting rights to:",
"brightness__title": "Set brightness",
"brightness__title": "Change display brightness",
Copy link
Member

Choose a reason for hiding this comment

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

@obrusvit do you know whether we need to do anything when an english string changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. In this case, it seems unnecessary. WDYT @Hannsek

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is not a big pain, please change it. 🙏🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need new translations, for sure. But we are adding new strings anyway, so I think it's no big deal.

@mmilata
Copy link
Member

mmilata commented Jul 9, 2024

Don't know if you know but you can take screenshots by pressing p in the emulator, they're saved under core/src/.

@ibz
Copy link
Contributor Author

ibz commented Jul 9, 2024

Code-wise this looks good, however the appearance is different from figma. Perhaps it changed in the meanwhile? It has orange heading while the finished flows have green so maybe it's still not final?

I see, indeed Figma changed since last time I looked. I remember @Hannsek said I should just use slider instead of the +/- buttons (which were in Figma), but there was no design for that at the time. Now there is, but with a different kind of slider.

I can wait until this stabilizes (green heading?)...

@Hannsek
Copy link
Contributor

Hannsek commented Jul 10, 2024

Sorry for that, here is the latest design. It uses the component that @obrusvit created.

Does that seem ok?

@ibz
Copy link
Contributor Author

ibz commented Jul 15, 2024

New screenshots here @Hannsek :

emu00000000
emu00000001
emu00000002
emu00000003
emu00000004
emu00000005

@Hannsek
Copy link
Contributor

Hannsek commented Jul 15, 2024

Thanks, looks better now. But please make sure to have the right copy on right places and the right dimensions according to the design.

@ibz ibz force-pushed the ibz/20240702-brightness branch from 7bcd42d to 06b3425 Compare July 15, 2024 09:44
@ibz ibz force-pushed the ibz/20240702-brightness branch from 06b3425 to 5acbac7 Compare July 17, 2024 07:46
@ibz ibz force-pushed the ibz/20240702-brightness branch from 5acbac7 to 3786874 Compare July 17, 2024 09:25
mmilata
mmilata reviewed Jul 17, 2024
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Almost ready to merge 👍

vendor/micropython Outdated Show resolved Hide resolved
core/embed/rust/src/ui/model_mercury/component/number_input_slider.rs Outdated Show resolved Hide resolved
mmilata
mmilata approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Please squash your fixups and then merge if CI passes.

@ibz ibz force-pushed the ibz/20240702-brightness branch from cf5be6f to 1e6439f Compare July 17, 2024 13:12
@ibz
Copy link
Contributor Author

ibz commented Jul 17, 2024

Monero test failed. Ran it separately and it passed. Merging...

@ibz ibz merged commit 7d90552 into main Jul 17, 2024
136 of 138 checks passed
@ibz ibz deleted the ibz/20240702-brightness branch July 17, 2024 14:10
@ibz ibz mentioned this pull request Jul 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obrusvit obrusvit obrusvit left review comments

@Hannsek Hannsek Hannsek left review comments

@mmilata mmilata mmilata approved these changes

Assignees

@ibz ibz

Labels
None yet
Projects
Firmware
Status: 🤝 Needs QA
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Brightness settings flow
4 participants
@ibz @Hannsek @mmilata @obrusvit