-
Notifications
You must be signed in to change notification settings - Fork 334
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 tab focus state #1496
Update tab focus state #1496
Conversation
ab5846d
to
a7aa551
Compare
c49ae8b
to
c156156
Compare
c156156
to
87e8be0
Compare
87e8be0
to
1deeb71
Compare
b811f6f
to
b7a9d5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some nice tidy up too.
Will need a release note update so is blocked on #1492
Suggested release note:
|
This was added in b29b698 but is not used anywhere – there’s nothing in the Javascript to apply the aria-current attribute.
Avoids multiple :focus rulesets, and applies the additional outline focus states only when the tab interface is used in IE8 (i.e. when JS is enabled) as that’s the only time its relevant – this means that the no-JS experience is consistent with how we style links elsewhere. This also defines the focus state outside of the `--selected` modifier because there are weird edge cases where a tab can be focused but not selected (e.g. moving from the mobile view to the desktop view with a link other than the current tab focused)
Avoid setting the font on the entire tab panel content; replace styling of links with `govuk-colour(“black”)` and `$govuk-text-colour` with the `govuk-link-style-text` mixin.
Rather than hardcoding to white, which _happens to be_ the background colour.
The ‘tab styling’ is currently applied to the link, with the list item doing very little in terms of presentation. This is problematic when it comes to presenting the focus state, which we want to be tight around the tab text, and thus visually smaller than the tab. By moving the tab styling to the list item, we can then make the link element smaller, and use an `::after` pseudo element to make the target area for the link fill the entire tab.
This avoids an incorrect aria-selected attribute being applied to links in the ‘contents’ if you started on a desktop layout with tabs and then resize or rotate your device and end up in the mobile layout.
Fixes the ‘–‘ in the `:before` pseudo-element on the mobile/non-js layout not using the font.
We get this for free from the link.
b7a9d5c
to
c613156
Compare
Rebased and release note added. |
@@ -207,6 +207,22 @@ If there are links back to radios or checkboxes components in your error summary | |||
|
|||
[Pull request #1426: Make radios and checkboxes components easier to link to from error summary](https://github.com/alphagov/govuk-frontend/pull/1426) | |||
|
|||
### Update the markup for tabs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. @m-green what do you think?
CHANGELOG.md
Outdated
@@ -207,6 +207,22 @@ If there are links back to radios or checkboxes components in your error summary | |||
|
|||
[Pull request #1426: Make radios and checkboxes components easier to link to from error summary](https://github.com/alphagov/govuk-frontend/pull/1426) | |||
|
|||
### Update the markup for tabs | |||
|
|||
You do not need to do anything if you're not using using Nunjucks macros. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also say that the user doesn't need to do anything if they're not using tabs, or are we taking that as a given?
(We didn't seem to do it anywhere else)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People seem to search for things and check if they have them, and ignore things they dont use, that we saw...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep we should be ok not mentioning it. (Unless govuk-tabs__tab--selected
appears in other non-tab components, but given the name I guess it wouldn't?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few style tweaks, and a question about line 214.
@@ -207,6 +207,22 @@ If there are links back to radios or checkboxes components in your error summary | |||
|
|||
[Pull request #1426: Make radios and checkboxes components easier to link to from error summary](https://github.com/alphagov/govuk-frontend/pull/1426) | |||
|
|||
### Update the markup for tabs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this heading still be correct?
### Update the markup for tabs | |
### Update the classes in tabs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or 'Update tab classes'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of… the class has also moved onto another element which makes it slightly more than just updating classes? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, good point. How about just 'Update tabs'? Or stick with the original - this one's definitely not a dealbreaker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case I think I'll leave it as-is 👍
CHANGELOG.md
Outdated
@@ -207,6 +207,22 @@ If there are links back to radios or checkboxes components in your error summary | |||
|
|||
[Pull request #1426: Make radios and checkboxes components easier to link to from error summary](https://github.com/alphagov/govuk-frontend/pull/1426) | |||
|
|||
### Update the markup for tabs | |||
|
|||
You do not need to do anything if you're not using using Nunjucks macros. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep we should be ok not mentioning it. (Unless govuk-tabs__tab--selected
appears in other non-tab components, but given the name I guess it wouldn't?)
CHANGELOG.md
Outdated
@@ -207,6 +207,22 @@ If there are links back to radios or checkboxes components in your error summary | |||
|
|||
[Pull request #1426: Make radios and checkboxes components easier to link to from error summary](https://github.com/alphagov/govuk-frontend/pull/1426) | |||
|
|||
### Update the markup for tabs | |||
|
|||
You do not need to do anything if you're not using using Nunjucks macros. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not need to do anything if you're not using using Nunjucks macros. | |
You do not need to do anything if you are not using Nunjucks macros. |
CHANGELOG.md
Outdated
|
||
You do not need to do anything if you're not using using Nunjucks macros. | ||
|
||
If you are not using the Nunjucks macros, remove the `govuk-tabs__tab--selected` from the first tab's link, and add the new class `govuk-tabs__list-item--selected` to its parent list item instead: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a style tweak. I've assumed this should say if you are using Nunjucks macro, because line 212 already says 'if you are not'.
If you are not using the Nunjucks macros, remove the `govuk-tabs__tab--selected` from the first tab's link, and add the new class `govuk-tabs__list-item--selected` to its parent list item instead: | |
If you're using Nunjucks macros, remove the `govuk-tabs__tab--selected` class from the first tab's link, then add the `govuk-tabs__list-item--selected` class to the link's parent list item. | |
For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh! It's entirely the other way around, good spot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 212 also says 'using using'!
c613156
to
35382a6
Compare
35382a6
to
de51a68
Compare
The ‘tab styling’ is currently applied to the link, with the list item doing very little in terms of presentation.
This is problematic when it comes to presenting the focus state, which we want to be tight around the tab text, and thus visually smaller than the tab. Our current approach, which creates the focus state using an
::after
pseudo-element, doesn't render correctly in IE 11.By moving the tab styling to the list item, we can then make the link element smaller, and use an
::after
pseudo element to make the target area for the link fill the entire tab (the 'pseudo-content trick').This also:
aria-current
ruleset in the Sassgovuk-tabs
)aria-selected
attribute when moving to and from mobileFixes #1472. I suggest reviewing commit-by-commit.
Screenshots
Chrome (macOS)
Safari (macOS)
Firefox (macOS, custom colours)
IE 8
IE 9
IE 10
IE 11