Fix Notice component action button margins #69430
Conversation
t-hamano
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Hi @daledupreez, |
Infinite-Null
force-pushed
the
fix/notice-component-action-spacing
branch
from
0f81665 to
c7db702
Compare
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Thanks for submitting this fix, and apologies for the delay in review. This feels like the right fix for a pretty obviously-broken appearance of actions. I'm going to do a bit more testing before approving, but in general the changes are looking good.
|
|
||
| // When it has better support, this can be replaced | ||
| // with column-gap since these are flex items. |
There was a problem hiding this comment.
We can remove this comment now that we're using gap.
| // When it has better support, this can be replaced | |
| // with column-gap since these are flex items. |
| // When it has better support, this can be replaced | ||
| // with column-gap since these are flex items. | ||
| margin-right: $grid-unit-10; | ||
| margin-top: $grid-unit-10; |
There was a problem hiding this comment.
Part of me feels it'd be better to have the margin apply to the container of the actions, although based on the current implementation we're aways rendering the actions container even if there's no actions, and we wouldn't want the extra margin if there's no actions.
In my opinion we probably shouldn't be rendering that container if there's no actions and then it'd be fine to apply the styling, but that's a bit more extensive change.
|
Curious why you chose to remove the CHANGELOG entry in 38fe1a4 ? It seems like it'd be a nice bug fix to highlight. |
| // When it has better support, this can be replaced | ||
| // with column-gap since these are flex items. | ||
| margin-right: $grid-unit-10; | ||
| margin-top: $grid-unit-10; |
There was a problem hiding this comment.
Referencing the Figma designs for this component, I think this margin should be $grid-unit-15 (12px) instead of $grid-unit-10 (8px), consistent with the gap between each action button.
| margin-top: $grid-unit-10; | |
| margin-top: $grid-unit-15; |
aduth
force-pushed
the
fix/notice-component-action-spacing
branch
from
38fe1a4 to
8606b17
Compare
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
I pushed a couple changes to your branch from the feedback I had, but I think this is looking great 👍
@Infinite-Null How do you feel about the additional changes I included, notably around only rendering the actions container when there are actions to be shown?
t-hamano
left a comment
t-hamano
left a comment
There was a problem hiding this comment.
This PR works fine for the Notice component alone, but in the NoticeList component, the spacing is larger than intended.
For example, open a post editor and run the following code in your browser console:
Details
wp.data.dispatch( 'core/notices' ).createNotice( 'warning', 'Post published.', {
isDismissible: true,
actions: [
{
label: 'Click me!',
variant: 'primary',
},
{
label: 'Or click me instead!',
},
{
label: 'Or visit a link for more info',
url: 'https://wordpress.org',
variant: 'link',
},
{
label: 'Click me!',
variant: 'primary',
},
{
label: 'Or click me instead!',
},
{
label: 'Or visit a link for more info',
url: 'https://wordpress.org',
variant: 'link',
},
],
} );trunk
This PR
I think we can now remove the style here as well:
gutenberg/packages/components/src/notice/style.scss
Lines 90 to 94 in 48a6e8a
Align to Figma designs
Apply margin styling on container, while avoiding margin affecting layout if there are no actions
aduth
force-pushed
the
fix/notice-component-action-spacing
branch
from
d18fc81 to
01fee66
Compare
|
Good catch @t-hamano . I removed that spacing style in 1713253 (and additional unnecessary styling in 01fee66). I also updated the Storybook stories for the
|
t-hamano
left a comment
t-hamano
left a comment
There was a problem hiding this comment.
LGTM! It looks good on the post editor as well.
| Before | After |
|---|---|
 |
 |
|
Thanks @Infinite-Null for the fix, and @t-hamano for the review 🙌 |
3 participants
Closes: #69426
What?
Improves spacing in Notice component between content and action buttons.
Why?
The spacing between the main content and action buttons in the Notice component was too tight, and there was no vertical spacing between multiple notice actions.
Screenshots: