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

Create contents list component #420

Merged
merged 9 commits into from Aug 8, 2017
Merged

Create contents list component #420

merged 9 commits into from Aug 8, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Aug 1, 2017

Marked as do not merge while I double check cross browser. Ready for review.

Notes to reviewers:

Changes:

  • Switch all formats to use contents list component
  • Consolidates specialist documents with same pattern as other formats
  • Remove analytics tracking on contents lists, currently unused
  • Remove unused templates
  • Incorporates @andysellick’s contents list suggestions in this PR: #394

contents list component - government frontend component guide 20170801

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-420 Aug 1, 2017 Inactive
- accept focus
- be focusable with a keyboard
- be usable with a keyboard
- indicate when it has focus

This comment has been minimized.

@vanitabarrett

vanitabarrett Aug 3, 2017
Contributor

How about "must inform the user how many items are in the list"?

This comment has been minimized.

@fofr

fofr Aug 7, 2017
Author Contributor

Added

`format_numbers` option will pull out numbers in the link text to render them as though they were the list style type. Applies to
numbers at the start of text, with or without a decimal. See the [format complex numbers fixture](/component-guide/contents-list/formats_complex_numbers) for details.
accessibility_criteria: |
The contents links must:

This comment has been minimized.

@vanitabarrett

vanitabarrett Aug 3, 2017
Contributor

Should this be 'contents list'? Or are you writing criteria for links within the list? If so, maybe 'contents list links' might make that clearer.

This comment has been minimized.

@fofr

fofr Aug 7, 2017
Author Contributor

Updated

@fofr fofr force-pushed the contents-list-components branch from cb2c32d to 1b5476c Aug 3, 2017
@fofr fofr temporarily deployed to government-frontend-pr-420 Aug 3, 2017 Inactive
else
'dashed'
end

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

Unneeded empty line? Sorry, nit picking...

This comment has been minimized.

@fofr

fofr Aug 7, 2017
Author Contributor

Fixed


%>
<% if contents.any? %>
<nav role="navigation" class="app-c-contents-list">

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

I fear I'm going to kick off a long discussion here but here goes. Not sure a nav element needs a role='navigation' - from the W3C validator, https://validator.w3.org/nu/?acceptlanguage=&doc=http%3A%2F%2Fgovernment-frontend-pr-420.herokuapp.com%2Fcomponent-guide%2Fcontents-list

This comment has been minimized.

@fofr

fofr Aug 7, 2017
Author Contributor

Some guidance here:
https://accessibility.blog.gov.uk/2016/05/27/using-navigation-landmarks/

We know that some people use the GOV.UK website with older browsers and screen readers, so we use both the HTML5 elements and ARIA roles to make sure the landmarks are available to as many people as possible. A good example is Internet Explorer 11, which does not have accessibility support for the HTML5 elements used to provide landmarks, but which does support the equivalent ARIA roles.

<% contents.each do |contents_item| %>
<li
class="app-c-contents-list__list-item app-c-contents-list__list-item--<%= parent_list_item_modifier %>"
>

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

Do we need both these classes on here, or could we combine them and just have the second one, with the modifier?

This comment has been minimized.

@fofr

fofr Aug 4, 2017
Author Contributor

I wanted to avoid overriding default styles in the modified class.

padding-right: $contents-spacing;

&:before {
content: "";

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

Currently Voiceover on OS is reading pseudo elements aloud, so each link text is preceded by "em dash". It's not ideal, but I've had a quick look and getting screen readers to ignore pseudo elements isn't super easy. I wonder if there's an alternative using a 0.5em height element with a top border?

Regardless of the outcome, this feels like something we should discuss with regard to the accessibility criteria for this component. And apologies, you didn't write this, it was already here.

This comment has been minimized.

@nickcolley

nickcolley Aug 3, 2017
Contributor

Since we have control over the markup, there's no reason why we couldnt put this inside the template and use aria-hidden?

This comment has been minimized.

@fofr

fofr Aug 7, 2017
Author Contributor

Tried doing this quickly. It's a little fiddly. We want the em dash to be in the top left and not have text wrapping beneath it, which means wrapping it in a span. Then we need some logic to include/exclude that span based on whether dashes are being used. It felt wrong moving inheritance out of the stylesheet.

- accept focus
- be focusable with a keyboard
- be usable with a keyboard
- indicate when it has focus

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

Should we also have a criteria about informing the user when the list contains a sub-list? I don't think it's something we'll need to add to the markup, Voiceover already announces that there's another list within a list.

formats_complex_numbers:
format_numbers: true
contents:
- href: "#"

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

Is there a reason that all the other fixtures have a full href value like #first-thing but the ones in this one are just #?

This comment has been minimized.

@fofr

fofr Aug 4, 2017
Author Contributor

No reason.

- href: "#"
text: 2017 four digit numbers ignored
- href: "#"
text: "2001: A space odyssey"

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

😂

assert_select "#{nested_link_selector} .app-c-contents-list__number", count: 0
assert_select "#{nested_link_selector} .app-c-contents-list__numbered-text", count: 0
end
end

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

Do we need an explicit test for calling the component with format_numbers true when there are no numbers to format?

This comment has been minimized.

@fofr

fofr Aug 4, 2017
Author Contributor

I don't think so.

- href: "#second-thing"
text: No numbers here
- href: "#third-thing"
text: None here either

This comment has been minimized.

@andysellick

andysellick Aug 3, 2017
Contributor

Ideally in the component guide it would be good to show the component in a right to left context. Perhaps rather than having a specific fixture option for this we could have the gem do it automatically, so each example gets repeated in RTL? Although that might throw up problems if there are unique IDs in those examples, maybe link to it separately.

Not really a comment on this PR, admittedly, although this component would definitely benefit from such a feature.

This comment has been minimized.

@vanitabarrett

vanitabarrett Aug 3, 2017
Contributor

I think there should already be a story for this in the backlog/planning board? It came up when doing the translation component too.

@fofr fofr force-pushed the contents-list-components branch from 1b5476c to 5e3ae21 Aug 7, 2017
@fofr fofr temporarily deployed to government-frontend-pr-420 Aug 7, 2017 Inactive
@andysellick
Copy link
Contributor

@andysellick andysellick commented Aug 7, 2017

IE8 is doing a weird thing with the focus state for dashed items in the contents list. Only happens for list items with the dash. See screenshots. Not occurring in IE9. Dashed items on live site in IE8 currently appear as regular dotted list items.

Problem:

screen shot 2017-08-07 at 11 44 49

screen shot 2017-08-07 at 11 50 18

Not a problem:

screen shot 2017-08-07 at 11 51 20

@andysellick
Copy link
Contributor

@andysellick andysellick commented Aug 7, 2017

Firefox (latest version) doesn't underline some of the elements correctly in the focus state (not a big problem, just being thorough). Not a regression, currently happening on live.

Chrome (correct):

screen shot 2017-08-07 at 12 19 03

Firefox (um):

screen shot 2017-08-07 at 12 19 12

Firefox (live site):

screen shot 2017-08-07 at 12 27 23

Increasing the line height to 1.4em fixes this.

Also doesn't underline heading list items properly when focussed.

Chrome:

screen shot 2017-08-07 at 12 22 17

Firefox:

screen shot 2017-08-07 at 12 22 01

@andysellick
Copy link
Contributor

@andysellick andysellick commented Aug 7, 2017

Firefox (latest) does some more odd things on numbered items (only) when focussed. That white gap top and bottom seems to vary in width from item to item. Not a regression, already happening on live.

I think this is being caused by the line height on the spans being slightly different from that on the link - so the outer yellow border lifts away from the background colour on the focussed element. Try slowly decreasing the line height in the inspector to see it in action. If element is inline-block that fixes it, but I don't know what other impact that might have.

screen shot 2017-08-07 at 12 20 44

screen shot 2017-08-07 at 12 20 56

fofr added a commit that referenced this pull request Aug 7, 2017
Focus styles on IE8 and older include the left margin, creating an odd
white box with orange border around the em dash. Use inline-block and
vertical alignment to fix focus styles.

#420 (comment)
0632386
@fofr fofr temporarily deployed to government-frontend-pr-420 Aug 7, 2017 Inactive
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 7, 2017

@andysellick Pushed a fix for IE8 focus styles in d4c2044 if you want to re-test.

@fofr fofr changed the title [Do not merge] Create contents list component Create contents list component Aug 7, 2017
@fofr fofr temporarily deployed to government-frontend-pr-420 Aug 7, 2017 Inactive
@fofr fofr force-pushed the contents-list-components branch from c6286da to 42806c4 Aug 7, 2017
fofr added a commit that referenced this pull request Aug 7, 2017
Focus styles on IE8 and older include the left margin, creating an odd
white box with orange border around the em dash. Use inline-block and
vertical alignment to fix focus styles.

#420 (comment)
0632386
@fofr fofr temporarily deployed to government-frontend-pr-420 Aug 7, 2017 Inactive
@fofr fofr force-pushed the contents-list-components branch from 42806c4 to eb1a7d3 Aug 7, 2017
@fofr fofr temporarily deployed to government-frontend-pr-420 Aug 7, 2017 Inactive
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 7, 2017

Requires some fixes for print styles:

screen shot 2017-08-07 at 13 57 28

@fofr fofr temporarily deployed to government-frontend-pr-420 Aug 7, 2017 Inactive
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 7, 2017

fofr added 5 commits Jul 31, 2017
* Switch all formats to use contents list component
* Consolidates specialist documents with same pattern as other formats
* Remove analytics tracking on contents lists, currently unused
* Remove unused templates
Focus styles on IE8 and older include the left margin, creating an odd
white box with orange border around the em dash. Use inline-block and
vertical alignment to fix focus styles.

#420 (comment)
0632386
fofr added 4 commits Aug 7, 2017
Be clearer what the links must do and what the list must do
Components are isolated, their order of inclusion should not matter.
* Create contents list helper for styles shared between print and
screen (just the table layout styles for lists with formatted numbers)
* Disable default print styles
* Use our own indent to nest dashed items when printing
* Hide the default list item marker when printing for numbered and
nested lists
* Put print styles on all pages

Component guide requires fix before print styles are visible:
alphagov/govuk_publishing_components#19
* Bump govuk_publishing_components to 0.5.0
* Pick up print styles feature needed by contents-list component
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 8, 2017

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Aug 8, 2017

Checked the print styles, looks good 👍

@fofr fofr merged commit f715662 into master Aug 8, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
security/snyk No new vulnerabilities
Details
@fofr fofr deleted the contents-list-components branch Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.