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 a breadcrumb component #700

Merged
merged 3 commits into from Jan 13, 2016
Merged

Create a breadcrumb component #700

merged 3 commits into from Jan 13, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jan 8, 2016

Navigational breadcrumbs, showing page hierarchy.
Accepts an array of breadcrumb objects. Each crumb must have a title and a URL.

Create a breadcrumbs component based on 20505dd, existing markup, and existing styles in _header.scss. Includes a couple of changes:

  • Removes strong styles as those don’t appear in the markup
  • Tweak the styles for better display in IE8 and below

https://trello.com/c/lNdlHrdq/235-component-creation-breadcrumbs-medium

screen shot 2016-01-08 at 16 23 39

cc @dsingleton @steventux

Running:
`bundle exec rails generate govuk_component breadcrumbs`
@fofr fofr changed the title [DISCUSS] Create a breadcrumb component Create a breadcrumb component Jan 8, 2016
@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 8, 2016

Closing while fixing a display bug.

@fofr fofr closed this Jan 8, 2016
@fofr fofr reopened this Jan 8, 2016
@fofr fofr force-pushed the breadcrumb-component branch Jan 8, 2016
fofr added 2 commits Jan 7, 2016
* Base styles on 20505dd7a398ea4ff7b449598049cff108dabab9
* Remove `strong` styles as those don’t appear in the markup
* Also based on #global-header in `_header.scss`
IE8 doesn’t support last-child, that’s a CSS3 selector.

Switch around margin and padding, so that the image appears to the left
of each crumb, but it hidden on the first item. Tweak left margin so
that space left and right of divider is even.
@fofr fofr force-pushed the breadcrumb-component branch to bd74f85 Jan 8, 2016
@tombye
Copy link
Contributor

@tombye tombye commented Jan 8, 2016

I think there's a good case for marking this up as a <nav> list rather than an <ol> with the breadcrumbs role. I can't find any reference to the WAI ARIA role and the W3C have a breadcrumb pattern in the HTML spec which uses <nav>'s instead:

http://www.w3.org/TR/html/common-idioms.html#rel-up

It might also be worth extending it to use the rel="up" attribute to indicate hierarchy, like the pattern used here: http://www.uvd.co.uk/blog/accessible-breadcrumbs-using-aria/. We could change it to put the You are here: text in an aria-label attribute instead of the <p> (or <h2> in the W3C's example).

(Some detail on the use of rel="up" shown in that article: http://www.w3.org/TR/2011/WD-html5-20110113/links.html#link-type-up.)

Apart from that, our HTML styleguide mentions we should use the <strong> tag to emphasise the current location in navigation: https://github.com/alphagov/styleguides/blob/master/html.md#emphasis. This is from a discussion a while ago with @LJWatson so we should include her and @aduggin in these discussions.

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 8, 2016

@tombye This is a port of the way we are currently building our breadcrumbs on GOV.UK with an IE fix. It's been prioritised to prevent blocking migration work.

It has highlighted that we don't have enough research or evidence for how well this pattern works. I agree that we should look into the pattern more, and improve it, but it shouldn't hold up this PR. I've been discussing this with @gemmaleigh who has moved the pattern to govuk_frontend_toolkit.

Regarding the strong tag. The current implementation on GOV.UK is using it incorrectly, as it highlights the parent section rather than the current page. On GOV.UK the final crumb in the crumb trail is also missing.

@alextea
Copy link
Contributor

@alextea alextea commented Jan 8, 2016

Does the design and research fall under the remit of Finding Things? It's certainly something we are planning to look at.

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 8, 2016

A benefit of this PR is that we can soon start using components for breadcrumbs, which should make the iteration of breadcrumb design and markup easier.

@tombye
Copy link
Contributor

@tombye tombye commented Jan 8, 2016

@fofr great, I'll pursue this away from this PR. Thanks for the info!

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Jan 8, 2016

The important thing to decide in this PR is the API/arguments the component has. That is much harder to change once the component is being used in production. The internal implementation of the component can be iterated and improved much more easily.

All of the comments about accessibility and research are 💯, but shouldn't block getting the groundwork in :)

@aduggin
Copy link

@aduggin aduggin commented Jan 8, 2016

The w3c html5 recommendation is:
"Authors are encouraged to markup bread-crumb navigation as a list. The nav element can be used to mark the list containing links as being a navigation block."
http://www.w3.org/TR/html/common-idioms.html#rel-up

On the money advice website we used a nav with an aria-label as the container around an unordered list.
We also added structured data using http://data-vocabulary.org/Breadcrumb to get breadcrumbs in appearing in google.

<nav class="breadcrumbs" role="navigation" aria-label="breadcrumbs">
  <ul class="unstyled-list">
      <li class="breadcrumbs__item" itemscope="" itemtype="http://data-vocabulary.org/Breadcrumb">
        <a class="breadcrumbs__link" href="/en" itemprop="url" data-di-id="di-id-a7ca1417-f0be1748">
          <span itemprop="title">Home</span>
</a>      </li>
      <li class="breadcrumbs__item" itemscope="" itemtype="http://data-vocabulary.org/Breadcrumb">
          <span class="visually-hidden">&gt;</span>
          <span class="breadcrumbs__divider" aria-hidden="true"></span>
        <a class="breadcrumbs__link" href="/en/categories/debt-and-borrowing" itemprop="url" data-di-id="di-id-4bee99a0-6751a3b5">
          <span itemprop="title">Debt and borrowing</span>
</a>      </li>
      <li class="breadcrumbs__item" itemscope="" itemtype="http://data-vocabulary.org/Breadcrumb">
          <span class="visually-hidden">&gt;</span>
          <span class="breadcrumbs__divider" aria-hidden="true"></span>
        <a class="breadcrumbs__link" href="/en/categories/before-you-borrow" itemprop="url" data-di-id="di-id-4bee99a0-4de6da3c">
          <span itemprop="title">Before you borrow</span>
</a>      </li>
  </ul>
</nav>

Looks like schema.org is the way to do it now though:
https://developers.google.com/structured-data/breadcrumbs?rd=1
https://schema.org/BreadcrumbList

You can see the breadcrumb in the google results here (it isn't displaying all the items though):
https://www.google.co.uk/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=Do+you+need+to+borrow+money

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 12, 2016

Do we want our breadcrumbs to appear in Google search results which would replace the URL? Adding the schema markup seems easy enough – probably a separate PR.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Jan 12, 2016

In terms of the component API, and basic functionality, this LGTM and i'd be happy to merge as is.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Jan 12, 2016

Do we want our breadcrumbs to appear in Google search results which would replace the URL? Adding the schema markup seems easy enough – probably a separate PR.

If we applied this to the component (in a future PR), would we want to apply it to the current breadcrumb markup as well? A mixture of search result styles (some with URLs, some with crumbs) may be an issue.

dsingleton added a commit that referenced this pull request Jan 13, 2016
Create a breadcrumb component
@dsingleton dsingleton merged commit 12c72ba into master Jan 13, 2016
1 check passed
1 check passed
default "Build #751 succeeded on Jenkins"
Details
@dsingleton dsingleton deleted the breadcrumb-component branch Jan 13, 2016
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.