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

New start button icon #1329

Closed
wants to merge 15 commits into from
Closed

New start button icon #1329

wants to merge 15 commits into from

Conversation

aliuk2012
Copy link
Contributor

@aliuk2012 aliuk2012 commented May 9, 2019

Replacing two tones png file for an inline SVG.

Before

old-start-button

After

new-start-button

👉 Demo

TODO:

  • add changelog entry
  • test in all the browsers we support
  • test text size 400%

aliuk2012 added 9 commits May 9, 2019 07:30
Ordering of :focus and :hover pseudo selectors are important to ensure
focus replaces the hover state. The scenario is where a user might decide to
use keyboard nagivation and the mouse is left hovered over the button. The
button should use the focus state and not the hover state
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1329 May 9, 2019 11:01 Inactive
@aliuk2012 aliuk2012 changed the base branch from master to v3 May 9, 2019 11:02
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1329 May 9, 2019 11:25 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1329 May 13, 2019 08:30 Inactive
@@ -10,9 +21,14 @@
{% endif %}
{% endif %}

{% if params.isStartButton %}
{% set iconMarkUp = '<svg data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20" height="19px" width="30px"><path fill="currentColor" d="M7.5 0H0l10 10L0 20h7.5l10-10-10-10z"/></svg>' %}
Copy link
Contributor

@NickColley NickColley May 13, 2019

Choose a reason for hiding this comment

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

I think we can improve the inline SVG by doing the following:

  • remove 'data-name' attribute as it's not needed.
  • add .govuk-button__icon class instead of styling the 'svg' element, this'll make sure if anyone else wants to put svg icons into this button we won't style theirs unintentionally.
  • change height and width to be relative units (em), this'll make the icon scale alongside the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First two points have been done.

@NickColley
Copy link
Contributor

Screen Shot 2019-05-13 at 10 41 12

Worth checking the sizes for height and width so that they're consistent with the underlying icon, this'll make sure the padding for the button looks consistent with the previous version.

@include govuk-device-pixel-ratio {
background-image: govuk-image-url("icon-pointer-2x.png");
background-size: 30px 19px;
@include govuk-media-query($until: tablet) {
Copy link
Contributor

@NickColley NickColley May 13, 2019

Choose a reason for hiding this comment

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

Do we need to do this with a breakpoint?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1329 May 13, 2019 11:48 Inactive
@fofr
Copy link
Contributor

fofr commented May 13, 2019

Will there be a solid black chevron for a focussed start button?

Screen Shot 2019-05-13 at 13 42 39

@NickColley
Copy link
Contributor

@fofr yep :)

@aliuk2012 aliuk2012 added this to the v3.0.0 milestone May 13, 2019
@@ -10,9 +21,14 @@
{% endif %}
{% endif %}

{% if params.isStartButton %}
{% set iconMarkUp = '<svg class="govuk-button__start-icon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20" height="19px" width="30px"><path fill="currentColor" d="M7.5 0H0l10 10L0 20h7.5l10-10-10-10z"/></svg>' %}
Copy link
Contributor

@NickColley NickColley May 13, 2019

Choose a reason for hiding this comment

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

We should check if we need focusable=false on internet explorer here, since SVGs can be added to the tab order mistakenly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested this in IE and it doesn't appear to be an issue. I think this might because the svg is already in a focusable element like a and button

@aliuk2012
Copy link
Contributor Author

Closing in favour of #1341

@aliuk2012 aliuk2012 closed this May 15, 2019
@36degrees 36degrees deleted the new-start-button branch February 10, 2020 09:49
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.

4 participants