-
Notifications
You must be signed in to change notification settings - Fork 20
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 design of accordion component #1884
Conversation
ecf0427
to
99106df
Compare
da452b3
to
93f4b6b
Compare
334bc15
to
21feaf8
Compare
21feaf8
to
472173e
Compare
472173e
to
ad8fbc2
Compare
ad8fbc2
to
b6def75
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.
Couple of comments for the draft, looking good so far!
Few extra bits of feedback:
- You've included the js for the design system accordion but not the test. Could we add this in please?
- My step by step design updates are imminent and this should be applied to the accordion. I think this just translates to extra spacing and reduced border widths. I'm aware you're working with Mia on this 👍
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Outdated
Show resolved
Hide resolved
app/assets/javascripts/govuk_publishing_components/components/accordion.js
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Outdated
Show resolved
Hide resolved
app/assets/javascripts/govuk_publishing_components/components/accordion.js
Outdated
Show resolved
Hide resolved
app/assets/stylesheets/govuk_publishing_components/components/_accordion.scss
Outdated
Show resolved
Hide resolved
38102c8
to
a3c1582
Compare
A couple of visual things:
When hovering over the accordions section the heading text changes colour to a dark blue and the show/hide text doesn't; should both change colour together? |
+1 to all of these, the Design has been updated since this was initially raised so will all be addressed. |
9c94946
to
e5a807f
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.
I'm a big fan of the last couple of changes you've made, very nice. I reckon you could squash your commits together a bit before merging though. Also maybe wait to merge or check in govuk frontenders as I know @alex-ju is putting out a breaking release today and as this is quite delicate we may want to wait a bit.
Once this is out I'd like to confer on differences between this and the step by step and see if we can bring in some of your technical choices eg: the css chevron, possibly moving towards bringing these close together, as is the dream.
Farewell accordion country 😃
} | ||
|
||
// Reduce Chevron size | ||
.gem-c-accordion-nav__chevron { |
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.
I love this!!
Thanks @owenatgov
Will do, I thought to myself the differences might be a bit easier to review this way to start - then the squash™
Ah OK, will do - i'll ping and hold off for the moment.
Yup this works, whoop, leaving Accordion country, now boarding 😃 |
8407e30
to
de92f81
Compare
Porting from Design system, adjusting format of JS for gem, integrate missing polfill. Adding additional styling and adjustments to markup to alter Design. Defining nodeListForEach only used within accordion, restore naming conventions
Converting accordion to gem version, updating new Design within inline Chervon SVG icon, isolating with gem as much as possible
Visual structure changed to header > (summary copy) > button, adjusting JS for new layout. Add CSS icon, moving icon to right, place the toggle links in a wrapper so the chevron icon doesn't jump (left / right ) due to text width when toggled. Updating spacing, hover styles, add focus state on wider parent element. Alter labelling so "hide this section" is not read by AT a second time when referencing inner Content yet maintaining relationship between the header and the inner Content. Change gem naming more inline with current conventions. Updating docs, explaining what experimental means for context
More accurate a11y AC but not fully comprehensive. Update CHANGELOG.md
de92f81
to
6d4da1e
Compare
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
- Using "govuk-em" - Remove explicitly defined colour as inherited. Many users alter their browser settings - a user might zoom in or increase the size of the font to adjust a webpage for their needs. In order to more closely meet Accessibility Acceptable Criteria of "zoom-text-only 300%"[1] the icon within the Accordion has been adjusted to use relative[2] values utilising an existing mixin[5] This allows the icon to scale correctly as zoom via "text only" is being used. As it's a relative value (not a pixel) this is more inline with Responsive Design[3] principles and also more closely aligns with WCAG 1.4.4[4] and previous iteration[6] --- [1] https://support.mozilla.org/en-US/kb/font-size-and-zoom-increase-size-of-web-pages#w_how-to-only-change-the-size-of-the-text [2] https://thecssworkshop.com/lessons/relative-units [3] https://alistapart.com/article/responsive-web-design/ [4] https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-scale.html [5] https://github.com/alphagov/govuk-frontend/blob/2f96f0d2cf088058d2beac7112167605c815b4df/src/govuk/tools/_px-to-em.scss#L12 [6] alphagov/govuk_publishing_components#1884 --- Non exhausted examples of how to test this: Within FireFox: View > Zoom > Zoom text only Within Chrome https://support.google.com/chrome/answer/96810?hl=en
This reverts commit e85b465.
This reverts commit e85b465. Reverting previous work completed on the Accoridon that diverged from the Design System
What?
Porting the Design System's Accordion component into the gem and updating the Design to improve a11y and usability.
Why?
GOV.UK currently uses 3 different accordions on different parts of the site. Some of them are not as accessible as they could be and they are not consistent in the way they open and close (the language they use). This is a fail of WCAG SC 3.2.4.
This update is part of on-going work to improve and create a consistent design that might eventually feed into the Design System depending on further research / outcome of performance. (Ticket)
Visual Changes
Before (left) After (right)
Interactions demo
Assistive Technology Testing
Testing Videos, Notes & Discussion
Screen.Recording.2021-02-08.at.09.41.42.mov
(user adjusts colour reference videos)
https://user-images.githubusercontent.com/71266765/106749352-52d9f580-661e-11eb-85fc-1f78fb368f8e.mov
Screen.Recording.2021-02-03.at.09.22.26.mov
Screen.Recording.2021-02-03.at.09.18.04.mov
high-contrast-greyscale-mode.mov
inverted.mov
IE11
The ‘Open all’ button reads out as ‘Open all sections’ for screen readers. This is potentially confusing for users as the visual content is different to what screen readers read out.
After discussion with team it was noted that this was based on the tested step-by-step and while not perfect is not an WCAG fail.
The team decided this was not a WCAG concern.
300% zoom
Possible Improvements
The Chevron could offer a visual cue on hover (after team discussion it was decided the current the affordance is fine)
Once the Accordion is opened, some work was done to auto-focus the inner Content once expanded for AT, I removed it as part of this PR as out of scope.
Buttons receiving the “hand cursor” has been noted as not strictly best practice. Needs discussion.
Anything else
PR was raised as Draft as on-going Design iterations were on-going in parallel
Closed PR on collections to ensure Design updates cascade through to uses on other repos.
Step-by-step update also implements this new Design in parallel (to resolve WCAG 3.2.4)
Porting over from Design System
common.js
polyfill fornodeListForEach
Conversation about altering this referenced here:
Further conversations about this element within the Design System are happening here
There is also a different prototype here
Issue raised as a reminder to remove this Gem Isolation after Design system updates to this element to stay inline with current architecture
Gem Doc's were updated to explain what Experimental means