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

Make all notice-list notices the same min-height #16891

Merged
merged 7 commits into from
Aug 8, 2019

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Aug 2, 2019

Follow up from #16861.

This PR is an attempt to make sure that the height of all single-line notices in the notice list are the same, regardless of whether or not they have an action button. The main reason for fixing this is so that the x button will appear properly visually centered in both (check the first Before screenshot below to see an example).

Normally, the inline button pushes up the line height quite a bit, so in order to get these to match up, we need to compensate for that somehow.

If notices were always on one line, we'd just add some extra line height to match, and we'd be all set. But we don't want to add too much line height, because when things wrap onto a second line, it'll look weird. This PR is a little tacky, but it seeks out a middle ground: taller (but still acceptable) line height, and very slightly shorter button + button margins.

This approach feels a little hacky to me, so I think we should also consider other solutions. Flexbox may be a more robust solution, so I'll keep trying that out too.


Screenshots

Before:

Screen Shot 2019-08-02 at 2 25 49 PM

Screen Shot 2019-08-02 at 2 26 10 PM

After:

Screen Shot 2019-08-02 at 2 25 05 PM

Screen Shot 2019-08-02 at 2 24 32 PM

Testing

Pasting this into your console should produce two test messages:

wp.data.dispatch('core/notices').createNotice(
	'warning',
	'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.',
	{
		isDismissible: true,
		actions: [
			{
				onClick: () => console.log( 'test' ),
				label: 'View post'
			}
		]
	}
);
wp.data.dispatch('core/notices').createNotice(
	'success',
	'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.',
	{
		isDismissible: true,
	}
);

@kjellr kjellr added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. labels Aug 2, 2019
@kjellr kjellr requested a review from jasmussen August 2, 2019 18:35
@kjellr kjellr self-assigned this Aug 2, 2019
@senadir
Copy link
Contributor

senadir commented Aug 2, 2019

while the solutions seems to work, I strongly think we should do a flex based solution, I might give it a try Sunday

@karmatosed
Copy link
Member

I would be +1 for a flexbox option here, how did your experiment go @senadir ? For now, removing design feedback as so long as it actually levels the min-height there is no design impact.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Aug 5, 2019
@kjellr
Copy link
Contributor Author

kjellr commented Aug 5, 2019

Yes, please do try out a flexbox solution! I gave flexbox a brief try last week, but ran into some issues around the inline action buttons, and knowing when to wrap them. It could use another focused effort.

@kjellr
Copy link
Contributor Author

kjellr commented Aug 6, 2019

I've pushed a few commits that swap out the approach to use flexbox instead. It appears to work pretty well, but still needs some thorough testing.

@kjellr
Copy link
Contributor Author

kjellr commented Aug 7, 2019

Alright, I've tried this flexbox solution out in Safari, Chrome, and FF on the Mac, as well as IE11 on Windows 7. It seems relatively solid.

Would someone mind giving this a test + review?

Copy link
Contributor

@senadir senadir left a comment

Choose a reason for hiding this comment

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

I've tested this with all possible content variations and it seems to work fine, the code is not directly straightforward to understand but there's no hacks to be documented so I guess it's fine with type of complex layouts

@kjellr
Copy link
Contributor Author

kjellr commented Aug 8, 2019

Thanks @senadir — I've updated to include some code comments to help clarify what's happening.

@senadir
Copy link
Contributor

senadir commented Aug 8, 2019

tests seems to fail randomly, I've restarted them

@kjellr kjellr merged commit 1c77941 into master Aug 8, 2019
@kjellr kjellr deleted the try/consistent-min-height-for-notices branch August 8, 2019 15:28
@youknowriad youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Make all notice-list notices the same min-height.

* Undo all the absolute positioning changes.

This reverts commit ef2af48.

* Try using flexbox to properly center things.

* Minor: Adjust dismiss button margin.

* Add code comments.
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Make all notice-list notices the same min-height.

* Undo all the absolute positioning changes.

This reverts commit ef2af48.

* Try using flexbox to properly center things.

* Minor: Adjust dismiss button margin.

* Add code comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants