Fix: Add button styles to notice actions without url. Allow custom classes on notice actions. by jorgefilipecosta · Pull Request #13116 · WordPress/gutenberg

jorgefilipecosta · 2018-12-27T16:30:00Z

This PR applies the following changes:

Currently, if a notice action did not contained a URL the button was totally unstyled. Now we apply default button styles.
Although the documentation said actions: (array) an array of action objects. Each member object should contain a `label` and either a `url` link string or `onClick` callback function., we accepted actions with both an URL and an onClick event creating a normal link with programmatic actions which is not recommended because of a11y reasons as raised by @afercia. Now we accept only one of the properties if an URL exist onClick is ignored.
We now allow the use of custom classes, so, for example, one can have a programmatic action that is rendered on the dom using a button but make it look like a link by adding the is-link class.

How has this been tested?

I executed the following code on the console:

wp.data.dispatch('core/notices').createNotice( 'error', 'Normal onClick Action', { actions: [ {label: 'Action', onClick: () => { alert( 'learn' ) } } ] } );

wp.data.dispatch('core/notices').createNotice( 'error', 'Link style onClick Action', { actions: [ {label: 'Action', onClick: () => { alert( 'learn' ) }, className: 'is-link' } ] } );

wp.data.dispatch('core/notices').createNotice( 'error', 'Normal link', { actions: [ {label: 'Link', url:"http://www.wordpress.org" } ] } );

I observed the following notices were displayed:

afercia · 2018-12-28T08:10:49Z

 		margin-left: 4px;
 	}
+	&.is-default {
+		vertical-align: unset;


unset is not supported by IE11: maybe try initial (baseline) or remove it all together?

Hi @afercia thank you for looking into this PR and suggesting alternatives. I used your fist suggestion and now vertical-align is set to 'initial'. Removing the rule all together would make buttons inside notices not aligned with the notice content.

jasmussen · 2019-01-14T07:55:39Z

Nice work! Here's a GIF_

I'm not sure how this is compared ot master, it's very likely a vast improvement. If it is, probably good to get it in. However there are a couple of little visual glitches that could use polish if you have time:

the 2nd notice is taller than the others, because the "link button" is as tall as a button but doesn't appear to be. You can see it when you hover, and the link hover style shows how tall the button is.
The link hover style in the 2nd notice is off. It shows aspect of the button styles bleeding through it seems. The active or focus style of the 1st link is also showing a background.
The 3rd notice is very close — but the height of the button style makes the close button offset. This might very well be the same issue as 1, and it might not be something we want to fix, not sure.

Yeah maybe don't "fix" 3:

The X should be in the top right corner for more than 1 line of text, so it's probably correct as it is.

But still worth fixing the CSS bleed in the button that's styled as a link. Or maybe not? I know @aduth has some ideas for either retiring or changing the "is-link" style.

jorgefilipecosta · 2019-01-21T12:26:32Z

Hi @jasmussen I tried to address #1 and #2 issues.
Here is a screenshot of how it looks now:

jasmussen · 2019-01-21T12:37:37Z

Lovely, ship it!

At some point we should (I might take a stab) resurrect the CSS improvements from #12301, that were unrelated to legacy notices. That would fix the overlap in your screenshot.

jasmussen · 2019-01-21T12:39:28Z

I'm mobile at the moment but will try and test/actually approve the PR as soon as I can.

jasmussen

Looks good! Nice work:

aduth · 2019-01-21T15:32:19Z

 * `onRemove`: function called when dismissing the notice
 * `isDismissible`: (boolean) defaults to true, whether the notice should be dismissible or not
-* `actions`: (array) an array of action objects. Each member object should contain a `label` and either a `url` link string or `onClick` callback function.
+* `actions`: (array) an array of action objects. Each member object should contain a `label` and either a `url` link string or `onClick` callback function. A `className` property can be used in case custom classes should be used for the button styles. If `className` is passed default classes e.g: is-default, is-link etc are not automatically added.


We now allow the use of custom classes, so, for example, one can have a programmatic action that is rendered on the dom using a button but make it look like a link by adding the is-link class.

Extending this way seems like something which should be done through a property / prop, not by adding the class directly. To this point, I'm not sure we should be changing the behavior depending on whether a custom class is provided.

Hi @aduth, I updated the code right now we don't change the behavior when a custom class is added. But developers can still remove default classes by setting
noDefaultClasses to true.

The following code produces the same result as shown on the screens before.

wp.data.dispatch('core/notices').createNotice( 'error', 'Normal onClick Action', { actions: [ {label: 'Action', onClick: () => { alert( 'learn' ) } } ] } ); wp.data.dispatch('core/notices').createNotice( 'error', 'Link style onClick Action', { actions: [ {label: 'Action', onClick: () => { alert( 'learn' ) }, className: 'is-link', noDefaultClasses: true } ] } ); wp.data.dispatch('core/notices').createNotice( 'error', 'Normal link', { actions: [ {label: 'Link', url:"http://www.wordpress.org" } ] } );

…asses on notice actions.

…asses on notice actions. (#13116)

jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 3523f48 to 113816c Compare December 27, 2018 18:31

afercia reviewed Dec 28, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 113816c to 5522384 Compare January 2, 2019 19:04

jorgefilipecosta requested review from a team and jasmussen January 11, 2019 18:37

jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 5522384 to 4216fac Compare January 21, 2019 12:24

jasmussen approved these changes Jan 21, 2019

View reviewed changes

aduth reviewed Jan 21, 2019

View reviewed changes

danielbachhuber added [Package] Notices /packages/notices [Type] Enhancement A suggestion for improvement. labels Jan 24, 2019

danielbachhuber mentioned this pull request Jan 24, 2019

Allow inline links in Notice content #13463

Closed

jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 4216fac to 1f7b685 Compare January 24, 2019 10:35

gziolo added this to the 5.0 (Gutenberg) milestone Jan 25, 2019

Fix: Add button styles to notice actions without url. Allow custom cl…

71c4c4e

…asses on notice actions.

jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 1f7b685 to 71c4c4e Compare January 28, 2019 18:14

jorgefilipecosta merged commit 5bdc8c6 into master Jan 29, 2019

jorgefilipecosta deleted the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch January 29, 2019 10:06

This was referenced Feb 4, 2019

Bump plugin version to 5.0.0-rc.1 #13652

Merged

Bump plugin version to 5.0.0 #13689

Closed

Bump plugin version to 5.0.0 #13692

Merged

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Fix: Add button styles to notice actions without url. Allow custom cl…

db81de4

…asses on notice actions. (#13116)

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Fix: Add button styles to notice actions without url. Allow custom cl…

1a89876

…asses on notice actions. (#13116)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Add button styles to notice actions without url. Allow custom classes on notice actions.#13116

Fix: Add button styles to notice actions without url. Allow custom classes on notice actions.#13116
jorgefilipecosta merged 1 commit intomasterfrom
fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions

jorgefilipecosta commented Dec 27, 2018 •

edited

Loading

Uh oh!

afercia Dec 28, 2018

Uh oh!

jorgefilipecosta Jan 2, 2019

Uh oh!

jasmussen commented Jan 14, 2019 •

edited

Loading

Uh oh!

jorgefilipecosta commented Jan 21, 2019

Uh oh!

jasmussen commented Jan 21, 2019

Uh oh!

jasmussen commented Jan 21, 2019

Uh oh!

jasmussen left a comment

Uh oh!

aduth Jan 21, 2019

Uh oh!

jorgefilipecosta Jan 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

jorgefilipecosta commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How has this been tested?

Uh oh!

afercia Dec 28, 2018

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

jasmussen commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgefilipecosta commented Jan 21, 2019

Uh oh!

jasmussen commented Jan 21, 2019

Uh oh!

jasmussen commented Jan 21, 2019

Uh oh!

jasmussen left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jorgefilipecosta commented Dec 27, 2018 •

edited

Loading

jasmussen commented Jan 14, 2019 •

edited

Loading