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

TT: buttons logic with long text #3307

Merged
merged 3 commits into from
Oct 16, 2023
Merged

TT: buttons logic with long text #3307

merged 3 commits into from
Oct 16, 2023

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented Sep 27, 2023 •
edited
Loading

Fixes #2902, fixes #2888.

As SwipePage evolved it accumulated some features that are currently not used, mainly the ability to use anything as the control elements on the last page. Except hold-to-confirm button, which had to use SwipeHoldPage component instead. By hardcoding a pair of buttons and merging the hold-to-confirm logic the code is somewhat simplified, also I've renamed it to ButtonPage as swiping is not what most users will do.

UI diff | differing screens

2f7a53e7221e884b94055bee8cfe868a7f25ec3e883ba24a5c0a67e98d609899 2158529bac42f5f8b4af367a0516e140cc31a5cd6efd27d3e393e70eb8a2026f 13731772d3d85bcee5af6f33b86b26df9dbf035cc9ef1850fce19a88284277a3
83c1102db48bf1a534b8a45be5eafcef63e9368f366d8e45e4fa8aed5b3af14d a745ca00396c15ed6373172031aef98b6540a8134496bcb20f5567eeae494a32

Note: the following screen is in the UI diff a lot because the width of the left button changed from 53 to 56 as is in Figma.
Screenshot 2023-09-27 at 12-42-53 TT-device_tests-bitcoin-test_authorize_coinjoin py test_cancel_authorization

Also changes hold-to-confirm to draw loader over the screen title, fixes #3000.

Screenshot 2023-09-28 at 20-10-58 TT-click_tests-test_reset_bip39 py test_reset_bip39

@mmilata mmilata requested a review from Hannsek September 27, 2023 10:55
@mmilata mmilata self-assigned this Sep 27, 2023
@mmilata mmilata force-pushed the mmilata/buttons-long-text branch 2 times, most recently from 3c840e3 to a4415c3 Compare September 28, 2023 17:59
@mmilata mmilata marked this pull request as ready for review September 28, 2023 18:25
@mmilata mmilata requested review from grdddj and removed request for prusnak and matejcik September 28, 2023 18:25
@Hannsek
Copy link
Contributor

Hannsek commented Sep 29, 2023

Looks good. 👌🏻👌🏻👌🏻

grdddj
grdddj approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@grdddj grdddj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code-wise LGTM. I have some small nits, but they are not so important

(maybe also please include [no changelog] into the first commit, so the changelog checks are green)

core/embed/rust/src/ui/model_tt/layout.rs Outdated Show resolved Hide resolved
core/embed/rust/src/ui/model_tt/component/simple_page.rs Outdated Show resolved Hide resolved
core/embed/rust/src/ui/model_tt/component/page.rs Outdated Show resolved Hide resolved
@mmilata mmilata force-pushed the mmilata/buttons-long-text branch 2 times, most recently from d6c97b1 to aa6574e Compare September 29, 2023 17:17
@mmilata
Copy link
Member Author

mmilata commented Sep 29, 2023

@matejcik when you have time can you please take a look at 6069120 + aa6574e?

@mmilata mmilata requested a review from matejcik September 29, 2023 20:16
@mmilata mmilata force-pushed the mmilata/buttons-long-text branch 3 times, most recently from ea9a4d6 to 94aeeca Compare October 5, 2023 22:23
@mmilata
Copy link
Member Author

mmilata commented Oct 5, 2023

Moved the clearing logic to struct Root which is a struct Child wrapper. PTAL @matejcik

Btw not using struct Pad for this as #3292 introduces efficient screen clearing for some display hardware.

@Hannsek
Copy link
Contributor

Hannsek commented Oct 11, 2023

Is here any change that is related to Suite? e.g. change of the message sent to Suite…

@mmilata
Copy link
Member Author

mmilata commented Oct 11, 2023

No, should not affect Suite at all.

matejcik
matejcik reviewed Oct 11, 2023
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

it makes a lot more sense with Root 👍

core/embed/rust/src/ui/layout/obj.rs Show resolved Hide resolved
core/embed/rust/src/ui/model_tt/component/page.rs Outdated Show resolved Hide resolved
@mmilata mmilata force-pushed the mmilata/buttons-long-text branch from 3aff84f to 795648a Compare October 16, 2023 10:03
@mmilata mmilata force-pushed the mmilata/buttons-long-text branch from 795648a to 0c0a6c3 Compare October 16, 2023 10:52
@mmilata mmilata merged commit a353c35 into main Oct 16, 2023
8 checks passed
@mmilata mmilata deleted the mmilata/buttons-long-text branch October 16, 2023 11:18
@bosomt
Copy link

bosomt commented Oct 17, 2023

QA OK

Info:

  • Suite version: desktop 23.11.0 (40b035b2be45a4792adc9fb8373fdacff7b89bb3)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/23.11.0 Chrome/116.0.5845.188 Electron/26.2.1 Safari/537.36
  • OS: MacIntel
  • Screen: 1440x900
  • Device: Trezor T2T1 2.6.3 regular (revision f7aec0a)
  • Transport: BridgeTransport 2.0.33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grdddj grdddj grdddj approved these changes

@matejcik matejcik matejcik approved these changes

@Hannsek Hannsek Awaiting requested review from Hannsek

Assignees

@mmilata mmilata

Labels
None yet
Projects
Firmware
Archived in project
Milestone
No milestone
5 participants
@mmilata @Hannsek @bosomt @matejcik @grdddj