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(suite): redesign FW checks UI #16599

Merged
merged 1 commit into from
Jan 28, 2025
Merged

feat(suite): redesign FW checks UI #16599

merged 1 commit into from
Jan 28, 2025
+176 −44

Conversation

Lemonexe
Copy link
Contributor

@Lemonexe Lemonexe commented Jan 24, 2025 •
edited
Loading

Description

  • reenable FW hash check other-error UI only for dev env
  • 👻 modal:
    • all variants have different subtitle to convey which check & what failure reason
    • other error also different title, checklist and button color = warning
  • warning banner:
    • all variants have "Contact Trezor Support" button -> goto same URL as on 👻 modal
    • other-error has color: warning

Related Issue

Resolve #16407

Screenshots:

👻 modals

hash-mismatch
ghost hash
other-error
ghost other-error
revision check
ghost rev

Banners

hash-mismatch
banner hash
other-error
banner other-error
revision check
banner rev

@Lemonexe Lemonexe force-pushed the feat/FW-hash-check-other-error-UI branch from 66e82b0 to 8363b05 Compare January 27, 2025 12:53
@Lemonexe Lemonexe marked this pull request as ready for review January 27, 2025 13:06
peter-sanderson
peter-sanderson approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@peter-sanderson peter-sanderson 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.

LFTM, details on slack

@Lemonexe Lemonexe merged commit 1ec877a into develop Jan 28, 2025
33 checks passed
@Lemonexe Lemonexe deleted the feat/FW-hash-check-other-error-UI branch January 28, 2025 12:40
@Lemonexe Lemonexe mentioned this pull request Jan 28, 2025
Merged
vojtatranta
vojtatranta reviewed Jan 29, 2025
View reviewed changes
packages/suite/src/views/onboarding/steps/SecurityCheck/SecurityChecklist.tsx
>
{items.map((item, index) => (
<Row key={index} gap={spacings.xl}>
{item.icon}
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this caused problem and TS check didn't see it for some reason. In SecurityCheck there is checklistItems varaible, where icon is string. That's likely the reason. Coz string is a ReactNode.

Before, string was kinda correct here as it was the IconName.

<Icon size={24} name={item.icon} color={theme.legacy.TYPE_DARK_GREY} />

My proposal: enforce the Icon element here. I am not sure how to do it correctly. I can check if <Icon /> is its own Component type and require just it, or to remove string from the ReactNode type here.

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 figuring out proper fix here: #16709

Copy link
Contributor Author

@Lemonexe Lemonexe Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oops, I missed all those checklist definitions 🤦
Yeah, exclude string from ReactNode could work, but would need comment to explain why...
EDIT: ReactElement is more suitable..

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

@komret komret komret left review comments

@vojtatranta vojtatranta vojtatranta left review comments

@peter-sanderson peter-sanderson peter-sanderson approved these changes

@MiroslavProchazka MiroslavProchazka Awaiting requested review from MiroslavProchazka MiroslavProchazka is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Hash Check "other-error" UI
4 participants
@Lemonexe @vojtatranta @komret @peter-sanderson