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

Add 'secondary' and 'warning' action buttons #1207

Merged
merged 3 commits into from
Apr 24, 2019

Conversation

dashouse
Copy link

@dashouse dashouse commented Feb 20, 2019

This PR:

  • Adds a tint and shade mix function - This is more controllable than darken and lighten
  • Converts existing button to use shade (Therefore a slight change to the hover colour)
  • Adds a secondary button modifier
  • Adds a warning button modifier

Secondary
I need to consult with this design regarding WCAG 2.1 non-text contrast. The text and the shadow meet contrast which I believe will be enough (the background of the button does not).
screen shot 2019-02-20 at 08 46 47

Warning
screen shot 2019-02-20 at 08 47 01

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 February 20, 2019 09:08 Inactive
@dashouse dashouse added the awaiting triage Needs triaging by team label Feb 20, 2019
@joelanman
Copy link
Contributor

Related discussion on secondary buttons:

alphagov/govuk-design-system-backlog#154

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 February 20, 2019 10:15 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 February 20, 2019 10:46 Inactive
@timpaul timpaul added 🕔 hours A well understood issue which we expect to take less than a day to resolve. Priority: Low and removed awaiting triage Needs triaging by team labels Feb 20, 2019
@edwardhorsford
Copy link
Contributor

Looks good @dashouse.

I'd probably aim for the border bottom on the grey button to meet 4.5:1 contrast - so possibly using $govuk-secondary-text-colour. Right now using a picker on the image gives me 3.4:1.

@soniaturcotte
Copy link

Here is the code for the buttons we use on GOV.UK
https://govuk-publishing-components.herokuapp.com/component-guide/button

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 February 26, 2019 13:12 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 February 26, 2019 13:43 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 March 4, 2019 15:27 Inactive
@soniaturcotte
Copy link

Tiny nitpick but I don't think the outlined buttons need to have the thicker bottom border. On Content Publisher, we increased the height of the button so the total height would be the same as the filled version. And then had the same border all the way around.

@dashouse
Copy link
Author

Hey @soniaturcotte, there was some concern that against our text input style the only signifiers of the button were colour and centred text. This was an attempt to differentiate them a bit further but it's also just a best guess on our part.

button

@fofr
Copy link
Contributor

fofr commented Mar 11, 2019

@dashouse Do you have a screenshot of how these alternative buttons look alongside text inputs?

Edited: Screenshot from alphagov/govuk-design-system-backlog#154

@soniaturcotte
Copy link

@dashouse that seems quite speculative. We haven't not seen this be a problem in any of our research. I would push to start simple and just add other signifiers if it shows as a problem.

The last screenshot on my comment (last comment) in the backlog shows it with a text input @fofr

alphagov/govuk-design-system-backlog#154

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 March 12, 2019 13:15 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 April 3, 2019 14:44 Inactive
@dashouse dashouse changed the title Contribution - 'Secondary' and 'Destructive' action buttons Contribution - 'Secondary' and 'Warning' action buttons Apr 17, 2019
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 April 17, 2019 10:35 Inactive
@dashouse dashouse added ⚠️ high priority Used by the team when triaging and removed Priority: low labels Apr 17, 2019
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

These look ace @dashouse.

I've added a few comments. Apart from fixing the secondary button styles for links, they're mostly just suggestions 🙂

src/components/button/_button.scss Outdated Show resolved Hide resolved
src/components/button/_button.scss Show resolved Hide resolved
src/components/button/button.yaml Show resolved Hide resolved
src/helpers/_shade.scss Outdated Show resolved Hide resolved
src/helpers/_shade.scss Outdated Show resolved Hide resolved
src/helpers/_shade.scss Outdated Show resolved Hide resolved
src/helpers/_tint.scss Outdated Show resolved Hide resolved
src/helpers/_tint.scss Outdated Show resolved Hide resolved
src/helpers/_shade.scss Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 April 17, 2019 14:27 Inactive
$govuk-button-hover-colour: darken($govuk-button-colour, 5%);
$govuk-button-shadow-colour: darken($govuk-button-colour, 15%);
// Primary button variables
$govuk-button-colour: #00823b; // sass-lint:disable no-color-literals
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondered why we were using colour literals here? Should it not be a green from the colour platte?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, the buttons have been their own 'button colour' green for a long time. It's something we're planning to fix with the palette change, but it shouldn't change in this PR.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 April 18, 2019 13:22 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 April 18, 2019 13:37 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1207 April 18, 2019 13:39 Inactive
@36degrees
Copy link
Contributor

I think this is ready for review now – top work @dashouse 👍

@36degrees 36degrees marked this pull request as ready for review April 18, 2019 14:02
@36degrees 36degrees changed the title Contribution - 'Secondary' and 'Warning' action buttons Add 'secondary' and 'warning' action buttons Apr 18, 2019
Copy link
Contributor

@aliuk2012 aliuk2012 left a comment

Choose a reason for hiding this comment

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

Awesome work and I like how the new shade/tint mixins are name-spaced.

@aliuk2012 aliuk2012 added this to the Next milestone Apr 23, 2019
@36degrees 36degrees merged commit 799299f into master Apr 24, 2019
@36degrees 36degrees deleted the secondary-action-buttons branch April 24, 2019 09:52
@36degrees
Copy link
Contributor

Dangit, changelog.

@aliuk2012
Copy link
Contributor

Relates to alphagov/govuk-design-system-backlog#154

@aliuk2012 aliuk2012 mentioned this pull request Apr 25, 2019
aliuk2012 added a commit to alphagov/govuk-prototype-kit that referenced this pull request Apr 25, 2019
- Add new secondary and warning button variants

  ([PR #1207](alphagov/govuk-frontend#1207))

- Add new govuk-shade and govuk-tint functions for creating shades and tints of
  colours.

  ([PR #1207](alphagov/govuk-frontend#1207))

- Add support for custom row classes on the summary list component (including support for some rows without action links)

  ([PR #1259](alphagov/govuk-frontend#1259))

- Ensure fieldset never exceeds max-width

  This fix ensures that both WebKit/Blink and Firefox are prevented from expanding their fieldset widths to the content's minimum size.

  This was preventing `max-width: 100%` from being applied to select menus inside a fieldset.

  See discussion in ["Reset your fieldset"](https://thatemil.com/blog/2015/01/03/reset-your-fieldset/) and raised by [issue #1264](alphagov/govuk-frontend#1264)

  ([PR #1269](alphagov/govuk-frontend#1269))

- Add various fixes to the summary list component:

  1. Fixes the 1px row height change when borders are removed
  Padding is now adjusted by 1px instead

  2. Fixes the text alignment when the actions column isn't added
  So the key column always stays at 30% width

  ([PR #1259](alphagov/govuk-frontend#1259))

https://github.com/alphagov/govuk-frontend/releases/tag/v2.11.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ high priority Used by the team when triaging 🕔 hours A well understood issue which we expect to take less than a day to resolve.
Projects
Development

Successfully merging this pull request may close these issues.

10 participants