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

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

Merged
merged 1 commit into from Jan 29, 2019

Conversation

Projects
None yet
6 participants
@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Dec 27, 2018

Fixes: #13009

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:
screenshot 2018-12-27 at 16 29 20

@jorgefilipecosta jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 3523f48 to 113816c Dec 27, 2018

@@ -34,6 +34,9 @@
&.is-link {
margin-left: 4px;
}
&.is-default {
vertical-align: unset;

This comment has been minimized.

@afercia

afercia Dec 28, 2018

Contributor

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 2, 2019

Author Member

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.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 113816c to 5522384 Jan 2, 2019

@jorgefilipecosta jorgefilipecosta requested review from jasmussen and WordPress/gutenberg-core Jan 11, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 14, 2019

Nice work! Here's a GIF_

notices

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:

  1. 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.
  2. 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.
  3. 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:

screenshot 2019-01-14 at 08 54 22

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 jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 5522384 to 4216fac Jan 21, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Jan 21, 2019

Hi @jasmussen I tried to address #1 and #2 issues.
Here is a screenshot of how it looks now:
screenshot 2019-01-21 at 12 25 16

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 21, 2019

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

@jasmussen
Copy link
Contributor

jasmussen left a comment

Looks good! Nice work:

gif

@@ -31,4 +31,4 @@ The following props are used to control the display of the component.
* `status`: (string) can be `warning` (yellow), `success` (green), `error` (red).
* `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.

This comment has been minimized.

@aduth

aduth Jan 21, 2019

Member

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 24, 2019

Author Member

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" } ] } );

@jorgefilipecosta jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 4216fac to 1f7b685 Jan 24, 2019

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

@jorgefilipecosta jorgefilipecosta force-pushed the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch from 1f7b685 to 71c4c4e Jan 28, 2019

@jorgefilipecosta jorgefilipecosta merged commit 5bdc8c6 into master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/add-styles-to-non-link-actions-allow-custom-styles-on-notices-actions branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment