Skip to content

Conversation

@kjellr
Copy link
Contributor

@kjellr kjellr commented Jul 15, 2019

Currently, our Notice component ships with 1em of padding on the top and bottom of the content area:

Screen Shot 2019-07-15 at 8 16 25 AM

This results in a lot of extra room on the top and bottom of the text:

Screen Shot 2019-07-15 at 8 11 25 AM

This PR changes that spacing to $grid-size-small, to balance it out with the spacing on the sides, and to align to our standard spacing increments, instead of 1em:

Screen Shot 2019-07-15 at 8 12 57 AM

@kjellr kjellr added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Package] Notices /packages/notices labels Jul 15, 2019
@kjellr kjellr requested a review from mapk July 15, 2019 12:19
@kjellr kjellr self-assigned this Jul 15, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good, and I agree the padding is both too much and inconsistent with the rest of the admin.

I was curious where the original values might have come from, and I note that the rest of the admin uses a total of 1em margin for paragraphs in a notice:

https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/css/common.css#L1388

I don't know if we care to mimic this, or if it's necessary to adjust other style values here (there's default padding on the notice itself, in addition to the margin of the content).)

@chrisvanpatten
Copy link
Contributor

This has been bugging me for a while but I've been too busy to fix it, so I'm very happy to see this PR! Thanks @kjellr!

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Capture d’écran 2019-07-15 à 4 07 22 PM

I knew there was some back and forth here. It looks like this breaks the alignment of the "close" button in the regular notices of the editor.

Try

wp.data.dispatch('core/notices').createNotice(
		'info',
		'Post published.', // Text string to display.
		{
			isDismissible: true, // Whether the user can dismiss the notice.
			// Any actions the user can perform.
            actions: [
				{
					onClick: () => console.log( 'test' ),
					label: 'View post'
				}
			]
		}
	);

So that they're not the same height as the sidebar's nav.
@kjellr
Copy link
Contributor Author

kjellr commented Jul 16, 2019

@youknowriad good catch. I pushed a new change that sets the top/bottom margins to $grid-size when they're inside of the components-notice-list. This keeps that close button in the right space, and also ensures that notices up there are a bit taller than the Sidebar's tabbed nav area (I remember this being a concern before, since we don't want the notices to be interpreted as an extension of that bar):

Screen Shot 2019-07-16 at 8 33 55 AM

Screen Shot 2019-07-16 at 8 34 10 AM

Copy link
Contributor

@youknowriad youknowriad 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 the fix

Copy link
Contributor

@chrisvanpatten chrisvanpatten left a comment

Choose a reason for hiding this comment

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

Looks great! 🚢

@kjellr kjellr merged commit 62439f0 into master Jul 16, 2019
@kjellr
Copy link
Contributor Author

kjellr commented Jul 16, 2019

Thanks everyone!

@kjellr kjellr deleted the update/notices-top-bottom-margins branch July 16, 2019 13:33
@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Design Feedback Needs general design feedback. [Package] Notices /packages/notices [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants