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

Update footer links to use new focus style #1321

Merged
merged 2 commits into from
May 10, 2019
Merged

Update footer links to use new focus style #1321

merged 2 commits into from
May 10, 2019

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented May 8, 2019

V3 focus styles

footer-new-focus-styles

V3 focus styles with legacy mode turned on

footer-new-focus-styles-legacy

Pre-v3 focus styles

footer-old-focus-styles

Closes #1320

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1321 May 8, 2019 13:05 Inactive
@NickColley NickColley marked this pull request as ready for review May 8, 2019 13:47
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1321 May 8, 2019 14:22 Inactive
@NickColley NickColley added this to the v3.0.0 milestone May 8, 2019
@@ -44,7 +44,12 @@
}

.govuk-footer__link {
@include govuk-focusable-fill;
text-decoration: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dashouse are you happy for this change to be made in 'compatibility mode' as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout, will see what @dashouse thinks.

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.

Looks good – one minor comment, and want to check that Dave's happy with the text-decoration change being applied in 'compatibility mode'

&:focus {
color: $govuk-focus-text-colour;
}
@include govuk-focusable-text-link;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the positioning of this important (coming after the pseudo-classes)? If so, might be worth a comment as I think we generally try and do our includes together at the top of the ruleset.

Copy link
Contributor Author

@NickColley NickColley May 9, 2019

Choose a reason for hiding this comment

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

Yeah we tend to put focus styles at the end of the block, I would prefer to have the mixin only contain properties to avoid this confusion.

I've had to do this in a few places to make sure the focus is applied correctly without needing to undo CSS.

It's on my list of things to consider once we've done all the work and know what the mixins need to do...

@dashouse
Copy link

Sorry if this wasn't clear but the links that are inline within a paragraph should have an underline. In this case that would be Government Digital Service and Open Government Licence v3.0

Regarding the legacy mode, should probably keep the underlines if it's not too much trouble.

@NickColley
Copy link
Contributor Author

Spoke to people on Slack, since people using govuk_template wont be using this component, it seems like it's not a big importance to add the old behaviour.

@dashouse
Copy link

Did you manage to make the inline link change? the preview is dead now

@NickColley
Copy link
Contributor Author

Note that this style was updated in this pull request: #1491

@NickColley
Copy link
Contributor Author

NickColley commented Nov 4, 2019

Some context on why we removed underlines from @36degrees:

I believe the intention was to treat the links in the footer in the same way we treat other navigational elements – like the header, or in the case of the Design System the side navigation. Where the ‘thing’ is a ‘list of links’, the thinking is/was that they don’t need the underlining because there’s no wider context in which they need to be distinguished. The exception is any link that does appear within e.g. a paragraph, in which case it does need the underline to distinguish it. It has been questioned a few times on support recently though.

The benefit, I believe, is that it allows us to have a clear hover state for those links, which we didn’t previously have

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.

4 participants