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 start button icon #1341

Merged
merged 5 commits into from
May 20, 2019
Merged

Add new start button icon #1341

merged 5 commits into from
May 20, 2019

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented May 13, 2019

V3:
V3

V3 Focused:
V3 Focused

V3 Legacy mode:
V3 Legacy mode

V3 Legacy mode focused:
V3 Legacy mode focused

V2:
V2

V2 Focused:
V2 Focused

Changed colours:

Screen Shot 2019-05-15 at 12 54 14

Issues:

SVG:

Internet Explorer 8 does not support SVG and will therefore not display the icon. We did not feel that it warranted a fallback as it did not affect how the user would interact with the button and the icon was purely used for presentation.

Flexbox

As this implementation uses flexbox not all browsers support it. I've checked with @dashouse and this shouldn't be a problem as the icon will wrap instead of floating to the right side.

Example where browser doesn't support flexbox

Screen Shot 2019-05-15 at 09 50 20

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 13, 2019 17:31 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 13, 2019 17:32 Inactive
@NickColley
Copy link
Contributor Author

NickColley commented May 13, 2019

@aliuk2012 I've explored some changes in this branch, will chat with you when we're both in the office. 👍

Can't tell if it's just my eyes or if this version of the icon looks weird now, will have a look again tomorrow... Think I might have messed the path up...

{% if params.isStartButton %}
{% set iconHtml %}
<svg class="govuk-button__start-icon" xmlns="http://www.w3.org/2000/svg" width="17.5" height="19" viewBox="0 0 33 40" focusable="false">
<path d="M0 0h13l20 20-20 20H0l21-19z" fill="currentColor"></path>

Choose a reason for hiding this comment

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

Suggested change
<path d="M0 0h13l20 20-20 20H0l21-19z" fill="currentColor"></path>
<path d="M0 0h13l20 20-20 20H0l20-20z" fill="currentColor"></path>

Choose a reason for hiding this comment

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

Before
Screen Shot 2019-05-15 at 11 23 42

After
Screen Shot 2019-05-15 at 11 23 53

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 15, 2019 11:15 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 15, 2019 11:41 Inactive
@aliuk2012 aliuk2012 mentioned this pull request May 15, 2019
3 tasks
@aliuk2012 aliuk2012 changed the title New start button rebased New start button icon and styling May 15, 2019
@aliuk2012 aliuk2012 changed the title New start button icon and styling New start button icon May 15, 2019
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 15, 2019 11:51 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 15, 2019 12:14 Inactive
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Looks great. Left a couple of comments. Also noticed that the icon now sits next to the text on mobile whereas it used to sit to the right on the button. Not sure if this is intentional.

Before:
Screen Shot 2019-05-17 at 11 41 45
After:
Screen Shot 2019-05-17 at 11 45 46

.govuk-button__start-icon {
margin-top: -.125em;
margin-right: -.08333333333333333em;
margin-left: .4166666666666667em;
Copy link
Member

Choose a reason for hiding this comment

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

Could these use govuk-em?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I initially did this the icon scaled alongside the font size, but to keep it consistent with the current implementation it no longer does that.

So with that in mind we don't need to use relative units (em), so I've changed it back to pixels.

Choose a reason for hiding this comment

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

I think this should be:

margin-top: -3px;
margin-right: 0; (or not there)
margin-left: govuk-spacing(1);
@include govuk-media-query($from: desktop) {
    margin-left: govuk-spacing(2);
}

The spacing scale does not change below govuk-spacing(4) so this change from 1 to 2 would need to be done with media queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a lot of complex code when we could be using relative units there, let's go with that for consistency...

src/components/button/template.njk Show resolved Hide resolved
@aliuk2012 aliuk2012 added this to the v3.0.0 milestone May 20, 2019
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 20, 2019 09:43 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1341 May 20, 2019 10:09 Inactive
@NickColley
Copy link
Contributor Author

@hannalaakso there's not really a great way to keep the icon right aligned on smaller screens that I know of with flexbox, so this felt like an OK compromise for the benefits we get from the inline SVG.

@NickColley NickColley changed the title New start button icon Add new start button icon May 20, 2019
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Looks great 🙌 Nice work everyone

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.

5 participants