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

fix(suite): Make view only promo content screen more responsive #13415

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

hstastna
Copy link
Contributor

@hstastna hstastna commented Jul 18, 2024

Description

Use spacingsPx for some css padding/margin values instead, where possible, add some more small adjustments.

Related Issue

Resolve #13261

Screenshots:

Before:
promo_before

After:
promo_after

@hstastna hstastna force-pushed the fix/13261-view-only-promo-content branch from 97e0ce0 to c48bd06 Compare July 18, 2024 14:06
@hstastna hstastna marked this pull request as ready for review July 18, 2024 14:12
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.

Looks much better. Few comments and ready to go

packages/suite/src/views/view-only/MacWindow.tsx Outdated Show resolved Hide resolved
packages/suite/src/views/view-only/MacWindow.tsx Outdated Show resolved Hide resolved
@hstastna hstastna requested a review from jvaclavik July 18, 2024 15:05
@peter-sanderson
Copy link
Contributor

responsible -> responsive ? :)

Copy link
Contributor

@peter-sanderson peter-sanderson left a comment

Choose a reason for hiding this comment

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

👏 thanks for cleaning up my ugly CSS coding 🙇 😁

Comment on lines 155 to 160
const ViewOnlyButton = styled(Button)`
width: 50%;

@media (max-width: ${variables.SCREEN_SIZE.SM}) {
width: 100%;
}
Copy link
Contributor

@jvaclavik jvaclavik Jul 19, 2024

Choose a reason for hiding this comment

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

I'm thinking could we avoid overriding Button component? 🙏 We try to avoid overriding as much as possible, because it increases complexity and lowers predictability.

I think it should be possible to do it here with following changes:

  • ButtonsContainer should contain media queries logic and it should change flex-direction
  • Button component should have isFullWidth prop which is the same like width: 100%

WDYT?

Copy link
Contributor Author

@hstastna hstastna Jul 19, 2024

Choose a reason for hiding this comment

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

Of course I do know we try to avoid overriding components as much as possible. I wouldn't do it if I saw some nicer ways to achieve the expected look and behavior.

Suggested ButtonsContainer changes could work, but without adjusting Button itself here I'm not sure if we can achieve the expectations easily. The thing is that...

...with isFullWidth the button takes too much horizontal space for big screens (when used with css flex props we need to make the screen more responsive). We'd still need to specify its width to avoid that and when we do this, using isFullWidth becomes unnecessary, so I've removed that.

But let's take some more time to figure this out...

@hstastna hstastna changed the title fix(suite): Make view only promo content screen more responsible fix(suite): Make view only promo content screen more responsive Jul 19, 2024
@hstastna hstastna requested a review from jvaclavik July 22, 2024 08:37
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.

Nicely done! Works like a charm 👍

@hstastna hstastna force-pushed the fix/13261-view-only-promo-content branch from 4f6e0df to 7e4a244 Compare July 23, 2024 08:08
@hstastna hstastna merged commit 48af875 into develop Jul 23, 2024
24 checks passed
@hstastna hstastna deleted the fix/13261-view-only-promo-content branch July 23, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polish Welcome screen
3 participants