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

feat(core): show account info in ETH send/stake flow #4175

Merged
merged 2 commits into from
Sep 24, 2024
Merged

feat(core): show account info in ETH send/stake flow #4175

merged 2 commits into from
Sep 24, 2024

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Sep 12, 2024

Fixes #3536

@ibz ibz self-assigned this Sep 12, 2024
@ibz ibz requested review from obrusvit and removed request for prusnak and matejcik September 12, 2024 14:16
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 12, 2024 •
edited
Loading

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) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@ibz
Copy link
Contributor Author

ibz commented Sep 13, 2024

This depends on #4117 being merged, otherwise the account being shown is incorrect.

@ibz ibz force-pushed the ibz/20240912-account-info branch 2 times, most recently from b54ff62 to e5f67ff Compare September 17, 2024 06:17
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 17, 2024 •
edited
Loading

legacy UI changes device test(screens) main(screens)

@ibz ibz added the translations Put this label on a PR to run tests in all languages label Sep 17, 2024
@ibz
Copy link
Contributor Author

ibz commented Sep 17, 2024

In German the menu item is a bit long.

@Hannsek what do you suggest? Leave as it is? Or change to something like "Kontoinfo." or simply "Konto"?

image

@ibz
Copy link
Contributor Author

ibz commented Sep 17, 2024 •
edited
Loading

Same in French - in that case even the staking address is too long ...

Rename to "Addr. de staking" & "Compte"?

image

@Hannsek
Copy link
Contributor

Hannsek commented Sep 17, 2024

Do we shorten it like that somewhere else? If yes, let's do it according to your suggestion. If not, let's check with Pavel.

@ibz
Copy link
Contributor Author

ibz commented Sep 17, 2024

In Portuguese, "Staking address" is fine, but "Claim address" is too long. Also, account info is missing. I'll fix this similarly to what we decide for the others.

image

@ibz ibz force-pushed the ibz/20240912-account-info branch 10 times, most recently from 892d5ca to d202d9f Compare September 18, 2024 10:41
@ibz
Copy link
Contributor Author

ibz commented Sep 18, 2024

OK, I ended up changing a couple of more strings. I looked at all the UI diffs and I think all of them look better in the new version. I'll paste all changes to Pavel also in case he wants to double check.

@ibz ibz force-pushed the ibz/20240912-account-info branch 2 times, most recently from 10640e4 to 217fa7e Compare September 20, 2024 14:08
obrusvit
obrusvit reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

A few points:

core/src/apps/ethereum/helpers.py Show resolved Hide resolved
core/src/trezor/ui/layouts/tr/__init__.py Outdated Show resolved Hide resolved
obrusvit
obrusvit approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

utACK

@ibz ibz force-pushed the ibz/20240912-account-info branch from 5b267d4 to f3e0352 Compare September 24, 2024 07:33
@ibz ibz merged commit a59ded5 into main Sep 24, 2024
129 of 130 checks passed
@ibz ibz deleted the ibz/20240912-account-info branch September 24, 2024 08:57
@bosomt
Copy link

bosomt commented Sep 24, 2024

QA OK

Info:

  • Suite version: desktop 24.10.0 (55ac8cb8a4edc1ac546c163acb3482cf53c4e4f8)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/24.10.0 Chrome/124.0.6367.243 Electron/30.3.1 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T3T1 2.8.4 regular (revision ab96ce7)
  • Transport: BridgeTransport 2.0.33

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

@obrusvit obrusvit obrusvit approved these changes

Assignees

@ibz ibz

Labels
translations Put this label on a PR to run tests in all languages
Projects
Firmware
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Information about account missing when sending ETH.
4 participants
@ibz @Hannsek @bosomt @obrusvit