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 details script #610

Merged
merged 6 commits into from
Mar 26, 2018
Merged

Add details script #610

merged 6 commits into from
Mar 26, 2018

Conversation

alex-ju
Copy link
Contributor

@alex-ju alex-ju commented Mar 16, 2018

This PR:

  • adds details.polyfill.js from govuk_frontend_toolkit
  • rewrites the code in plain javascript and following the JavaScript style guide
  • relies on CSS to recreate the arrow when the details element it is not natively supported by the browser
  • add tests to make sure the script works as expected (regarding ARIA attributes)

Manual tests

Browsers

  • IE8+
  • Latest Safari, Chrome, Firefox, Edge
IE8

details-ie8-closed
details-ie8-opened

IE9

details-ie9-closed
details-ie9-opened

IE10 details-ie10-closed details-ie10-opened
IE11 details-ie11-closed details-ie11-opened
Firefox 58 with custom colours details-ff58-custom-colors-closed details-ff58-custom-colors-opened

AT testing

  • Custom colours
  • VoiceOver
    Notes:
    With details.jsit announces collapsed button/expanded button.
    Without details.js:
    • VoiceOver on Chrome announces disclosure triangle regardless of state
    • VoiceOver on Safari announces collapsed summary/expanded summary
  • JAWS17/18
    Notes:
    With details.js on IE11/Firefox52 announces button collapsed/expanded.
  • NDVA
    Notes:
    With details.js on Firefox52/IE11 announces button collapsed/expanded.
  • Dragon

Automated tests

  • Jest integration tests written

Trello card

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-610 March 16, 2018 14:25 Inactive
Remove the script that creates an additional element for the details arrow and rely on CSS to create the arrow. IE8 doesn't know :: or currentColor so we fallback on : and inherit respectively.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-610 March 17, 2018 15:54 Inactive
@alex-ju alex-ju changed the title [WIP] Add details script Add details script Mar 21, 2018
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.

Generally looking really good – the tests are good but I think there are a number of code paths that aren't being tested right now, and it'd be good to cover them.

expect(summaryRole).toBe('button')
})

it('should set the element controlled by the summary using aria-controls', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a test for a details element that already has an existing ID, to ensure the existing ID is used instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the id attribute to the details component, but what we're generating via JavaScript and using in ARIA attributes are the <summary> and <div> container ids; for which one can't pass any attributes at the moment - so a test won't be relevant in this cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha.

expect(summaryAriaExpanded).toBe('false')
})

it('should add a unique id to the hidden content in order to be controlled by the summary', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a test for a details element that already has an existing ID, to ensure the existing ID is not overriden.

expect(hiddenContainerAriaHidden).toBe('true')
})

it('should indicate the open state of the content', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a test for a details element that is open when the page loads.

Copy link
Contributor

Choose a reason for hiding this comment

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

(That would also need a test that aria-hidden is false)

Copy link
Contributor Author

@alex-ju alex-ju Mar 21, 2018

Choose a reason for hiding this comment

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

Added

  • the open attribute to the details component
  • an example where the details component is expanded by default
  • the associated tests for the expanded/opened by default example

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-610 March 21, 2018 15:54 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants