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 new focus style to buttons #1315

Merged
merged 4 commits into from
May 13, 2019
Merged

Add new focus style to buttons #1315

merged 4 commits into from
May 13, 2019

Conversation

aliuk2012
Copy link
Contributor

@aliuk2012 aliuk2012 commented May 3, 2019

Before:

image

After:

image

Browser testing:

  • Chrome 73 & 74
  • Firefox 65 & 66
  • OS Safari 10, 11, 12
  • Android Samsung Galaxy (Chrome)
  • iOS XS / 8 (Safari)
  • Edge 17 & 18
  • IE 8 - 11

Others tests:

Issues:

  • Start button icon is still a two toned white png.

👉 Before: https://govuk-frontend-review.herokuapp.com/components/button
👉 After: https://govuk-frontend-review-pr-1315.herokuapp.com/components/button
Closes #1305

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 3, 2019 10:56 Inactive
@aliuk2012 aliuk2012 marked this pull request as ready for review May 3, 2019 11:15
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 3, 2019 11:56 Inactive
@NickColley
Copy link
Contributor

Looking good so far, added some comments to make it more consistent with the rest of the codebase. 👍

@dashouse
Copy link

dashouse commented May 3, 2019

Looking good so far

The secondary :focus state that is applied at the same time as :active is causing the button to get smaller. I believe I may have added some padding on the prototype, ideally the button will remain the same size in every state.

It also appears to have lost the margin-top that is applied in this state to give the button it's "pushed down" feel

@36degrees
Copy link
Contributor

I don't think this is working correctly for secondary or warning buttons at the minute. They're not getting the yellow highlight, and in the case of the warning buttons the text colour is changing to black (on red)

@aliuk2012
Copy link
Contributor Author

@36degrees Ah yes I see that, I think thats because I removed the important and that was forcing everything to work. I'll take a look at it as @NickColley suggested perhaps moving the focus/hover state to the bottom

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 8, 2019 08:46 Inactive
@NickColley NickColley added this to the v3.0.0 milestone May 8, 2019
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 9, 2019 07:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 9, 2019 07:39 Inactive
@NickColley
Copy link
Contributor

It looks like we can make the highlight border thicker to be consistent with Dave's original design:

Screen Shot 2019-05-09 at 16 00 05

Original:

Screen Shot 2019-05-09 at 16 00 27

@NickColley
Copy link
Contributor

The original avoids the focus state when hovered, and active, which is not consistent in this version.

@NickColley
Copy link
Contributor

Buttons with modifiers have inconsistent focus state compared with the original.

Let me know if you wanna go over any of this feedback together.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 10, 2019 09:55 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 10, 2019 10:17 Inactive
@aliuk2012
Copy link
Contributor Author

It was decided that the final version of the buttons would not have a double thick border like links and the most noticeable changes would be the background colour changing to yellow and the bottom border colour changing to black

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

In Dave's original design, the focus style is not shown when the user is actively (:active) pressing the button, or hovering over the button (:hover).

In this iteration, active works but hover still shows the focus. You could consider something like :not(:active):not(:focus) maybe.

Additional to this, the secondary and warning buttons show the focus state when actively pressing the buttons.

src/components/button/_button.scss Outdated Show resolved Hide resolved
src/components/button/_button.scss Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 11:31 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 11:59 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 12:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 13:37 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 14:17 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 14:22 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 14:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 14:45 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1315 May 13, 2019 14:51 Inactive
@aliuk2012 aliuk2012 merged commit c7cc494 into v3 May 13, 2019
@aliuk2012 aliuk2012 deleted the new-focus-button branch May 13, 2019 15:27
tombye added a commit to alphagov/notifications-admin that referenced this pull request Sep 14, 2020
tombye added a commit to alphagov/notifications-admin that referenced this pull request Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

6 participants