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

[refs #384] Create summary list component from GOV.UK #404

Merged
merged 6 commits into from
Feb 27, 2019

Conversation

chrimesdev
Copy link
Member

@chrimesdev chrimesdev commented Feb 25, 2019

Description

Summary list component styles, markup, macro and documentation taken
from the GOV.UK Frontend.

To do

  • Design sign off (@davidhunter08)
  • BackstopJS tests
  • Screen resolution, browser and accessibility testing

@chrimesdev chrimesdev force-pushed the feature/summary-list-component branch 7 times, most recently from 3654bd3 to 330c7ea Compare February 25, 2019 10:36
@chrimesdev
Copy link
Member Author

chrimesdev commented Feb 25, 2019

@davidhunter08 would you be able to have a look at the design (spacing etc) and let me know if you're happy with it.

@GrilloPress if you want to have a look too and feedback anything 👍

@chrimesdev chrimesdev force-pushed the feature/summary-list-component branch from 330c7ea to 2bd48aa Compare February 25, 2019 10:59
Summary list component styles, markup, macro and documentation taken
from the GOV.UK Frontend.
@chrimesdev chrimesdev force-pushed the feature/summary-list-component branch from 2bd48aa to 0d0f333 Compare February 25, 2019 11:09
@davidhunter08
Copy link
Contributor

@AdamChrimes - Design (spacing etc.) looks ok.

@chrimesdev
Copy link
Member Author

Thanks @davidhunter08, @mcheung-nhs please could you have a look when you are back. 🌤

@chrimesdev chrimesdev marked this pull request as ready for review February 25, 2019 13:24
@GrilloPress
Copy link

Looks good generally. Big fan of the links going to the left-hand margin on mobile.

Two thoughts:

  1. Word break attribute seems to be causing some issues with the contact information label
    image

  2. Wrapping <dl> in <div>

Just working through some issues with GDS about this. Technically it may not be valid HTML to wrap the <dt> and <dd> inside a <div>.

Axe interprets the rule as a strict <dt> must have a parent of <dl>

I'm interpreting https://www.w3.org/TR/html5/grouping-content.html#the-dl-element as meaning that we can have <div> children of <dl> but waiting for GDS to get back to me.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dl#Wrapping_name-value_groups_in_%3Cdiv%3E_elements

@GrilloPress
Copy link

Dave House from GDS confirmed my interpretation of the HTML spec. From GDS point of view.

So wrapping<div> is fine. But will trigger some accessibility tools to raise an error. DAC also raised it to us but I'm challenging them on it.

Something to be aware of.

@joelanman
Copy link

Related work in Axe: dequelabs/axe-core#1284 (unreleased I think)

@mcheung-nhs
Copy link
Collaborator

I would also interpret the W3 specs mentioned that a <div> is allowed in a <dl>.
Also, running the code through the W3 HTML5 validator doesn't bring up any issues.

Looking at further discussions with the axe-core library (dequelabs/axe-core#262) it seems there are no issues from a screen reader point of view.

Therefore I'm happy for this to be merged and we'll have to put any exceptions if/when we implement axe in our CI.

@mcheung-nhs mcheung-nhs merged commit 01452d9 into master Feb 27, 2019
@mcheung-nhs mcheung-nhs deleted the feature/summary-list-component branch February 27, 2019 19:52
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.

5 participants