-
-
Notifications
You must be signed in to change notification settings - Fork 278
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
Refactor QrScannerModal
, remove unused allowPaste
prop
#14513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring 🙏 I like it
// eslint-disable-next-line local-rules/no-override-ds-component | ||
const ErrorTitle = styled(Paragraph)` | ||
text-align: center; | ||
color: ${colors.legacy.TYPE_RED}; | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
<Translation id="TR_GENERIC_ERROR_TITLE" /> | ||
</ErrorTitle> | ||
</Paragraph> | ||
<ErrorMessage>{error}</ErrorMessage> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure but I think it could be possible to replace also this one:
<Paragraph variant="destructive" align="center" margin={{ spacings.xxxl }}>...</Paragrap>
Why I'm not sure: Because there is a span but at the same time i think shouldn't be there because we use align.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using the NewModal component, we'd appreciate it:)
Okay I rewrote the whole thing. Now, if you can do me a favour... #14512 |
@@ -84,43 +30,17 @@ const StyledQrReader = styled(QrReader)` | |||
padding-top: initial !important; | |||
|
|||
& > video { | |||
border-radius: 16px; | |||
border-radius: ${spacingsPx.md}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use border radii variables from theme package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/rebase |
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/11029931486 |
d626538
to
b741a40
Compare
Description
Refactoring I made as a preparation for #13460. The
allowPaste
prop was not used. Removing some legacy styling.QA: Just refactoring and design update (see pictures below).