-
Notifications
You must be signed in to change notification settings - Fork 2k
MSD Account Notifications: Fix site card UI responsiveness #107454
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
MSD Account Notifications: Fix site card UI responsiveness #107454
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
| &__site-settings { | ||
| margin-left: -24px; | ||
| margin-right: -24px; | ||
| margin-inline-start: -$grid-unit-20; |
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.
This approach isn't ideal since it makes assumptions on the parent container's padding. Another solution would be to make the card padding-less and allow its children to set the padding instead, but I'm not convinced that the complexity is worth it 🤔
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.
Yeah, I think this is a pretty unique case.
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
| } | ||
|
|
||
| &__card-placeholder { | ||
| height: 112px; |
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.
Magic number?
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.
Yep, existing code. But it's for the placeholder, so not too worried about it if it minimizes layout shifts.
| } | ||
|
|
||
| &__url { | ||
| &__url a { |
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.
I see, so a.dataviews-url-field was applying the styles in this URL through this export.
Once the styles were moved into the dataviews, these stopped working (or identified how this wasn't working properly). 🤔
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.
Yes, but I've always thought of it as a hack so I don't mind losing this "convenience". We could create a more generic URL renderer component that not only styles the URL but also strips out the protocol, etc.
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.
Tested. Works.
I agree, this relies on knowing what the card's padding is at all times and that's not ideal.
However, I can't think of another way to do that without rewriting the CollapsibleCard component as you mention.
The Card component now has Logical padding properties but that doesn't help. If we could pass in CSS values, we would be able to create a CSS variable to use for this case... but that's not possible.
We do set a default of 16px for the body on mobile.
Could we make that a variable we use across the dashboard?
app/styles.scss
--mobile-padding-inline-default: #{$grid-unit-20}
That's true, the latest Card component supports padding
I think avoiding defining our own styles as much as possible is still a good rule of thumb. I'd love to get access, for instance, the token definitions here https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/card/get-padding-by-size.ts to that they can be passed down to children as mentioned above. Or maybe the simples solution is to just change the design so that the separator doesn't take the full width of the card 🙂 |
|
Merging for now, appreciate the brainstorm @StevenDufresne! |
Proposed Changes
This PR fixes the site card content overflowing on mobile resolution.
This PR also fixes the site URL being blueberry blue instead of gray.
Why are these changes being made?
Testing Instructions
Pre-merge Checklist