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.

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

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

komret
Copy link
Contributor

@komret komret commented Sep 24, 2024

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).

@komret komret added the code Code improvements label Sep 24, 2024
Copy link
Contributor

@jvaclavik jvaclavik left a 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

Comment on lines -58 to -62
// eslint-disable-next-line local-rules/no-override-ds-component
const ErrorTitle = styled(Paragraph)`
text-align: center;
color: ${colors.legacy.TYPE_RED};
`;
Copy link
Contributor

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>
Copy link
Contributor

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.

Copy link
Contributor

@seibei-iguchi seibei-iguchi left a 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:)

@komret
Copy link
Contributor Author

komret commented Sep 24, 2024

Okay I rewrote the whole thing. Now, if you can do me a favour... #14512

Before:
Screenshot 2024-09-24 at 19 08 49
Screenshot 2024-09-24 at 19 22 35

After:
Screenshot 2024-09-24 at 19 08 43
Screenshot 2024-09-24 at 19 21 37

@@ -84,43 +30,17 @@ const StyledQrReader = styled(QrReader)`
padding-top: initial !important;

& > video {
border-radius: 16px;
border-radius: ${spacingsPx.md};
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seibei-iguchi seibei-iguchi self-requested a review September 25, 2024 07:37
@komret
Copy link
Contributor Author

komret commented Sep 25, 2024

/rebase

Copy link

@komret komret merged commit 86af559 into develop Sep 25, 2024
23 checks passed
@komret komret deleted the fix/release-camera branch September 25, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants