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 breadcrumbs pattern #334

Merged
merged 7 commits into from
Jun 24, 2016
Merged

Add breadcrumbs pattern #334

merged 7 commits into from
Jun 24, 2016

Conversation

bjacobel
Copy link
Contributor

@bjacobel bjacobel commented Jun 9, 2016

Refs TNL-4599

Adds breadcrumb styling and pattern example. Uses JS components added to the UITK in #72.

http://ux-test.edx.org/bjacobel/breadcrumbs/

Testing Checklist

  • Manually test responsive behavior.
  • Manually test right-to-left behavior.
  • Manually test a11y support.

Non-testing Checklist

  • Consider any documentation your change might need, and which users will be affected by this change.

Post-review

  • Squash commits into discrete sets of changes with descriptive commit messages.

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

TODO before merge

  • Merge UITK PR first
  • release UITK to NPM
  • update the UXPL package.json git uri to point to npm UITK v 1.2.0.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @andy-armstrong, @talbs and @clrux to be potential reviewers

@@ -33,7 +33,7 @@
"css-loader": "~0.23.1",
"browser-sync": "*",
"del": "*",
"edx-ui-toolkit": "~0.10.0",
"edx-ui-toolkit": "git://github.com/edx/edx-ui-toolkit#bf34f8e6b0028b90c07536bd7279325ed6c3fa86",
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: don't merge until this can be changed to an actual release number.

@andy-armstrong
Copy link
Contributor

👍 Awesome stuff, @bjacobel. If it is straightforward I'd like to see a server-side rendered example, but if you prefer that can come in a follow-on PR.

@bjacobel bjacobel force-pushed the bjacobel/breadcrumbs branch from c222078 to 0b58961 Compare June 15, 2016 17:59
@downzer0
Copy link
Contributor

The last page in the breadcrumb shouldn't be a link. Maybe include a heading (it can be sr-only) for even more context.

The W3 breadcrumb "spec"
An example from the W3

@downzer0
Copy link
Contributor

Oh, also, if you use a list for the breadcrumb links, screenreaders announce the number of items, which could add additional clarity to how "deep" in they are.

@bjacobel bjacobel force-pushed the bjacobel/breadcrumbs branch from 0b58961 to 87cf94f Compare June 15, 2016 20:45
@bjacobel
Copy link
Contributor Author

@clrux Updated with some accessibility improvements.

@downzer0
Copy link
Contributor

@bjacobel Sorry man, after doing some additional testing, the nav only has one child (ul) and so doesn't properly announce the correct number of children. It'll always be one.

Check out this example, which I tested, and does work. https://codepen.io/jonneal/pen/ianKu

Turns out we don't really need a list if we're using the nav element.

@bjacobel
Copy link
Contributor Author

@clrux no worries! I've updated to remove the ul / li structure.

@bjacobel bjacobel mentioned this pull request Jun 21, 2016
1 task
@bjacobel bjacobel force-pushed the bjacobel/breadcrumbs branch 2 times, most recently from 77ca03d to 962bc23 Compare June 21, 2016 20:28
@bjacobel bjacobel mentioned this pull request Jun 21, 2016
11 tasks
@bjacobel bjacobel force-pushed the bjacobel/breadcrumbs branch from 962bc23 to d8de742 Compare June 22, 2016 21:48
@bjacobel
Copy link
Contributor Author

Example of pre-rendered breadcrumbs is live on doc site @andy-armstrong.

Checking Chris off for review as he's already done the a11y review when the components went into the toolkit.

@andy-armstrong
Copy link
Contributor

👍 looks awesome. Thanks for the pre-rendered version.

I recommend having @AlasdairSwan do the second review instead of @dianakhuang (Alasdair, is that okay with you?)

display: inline-block;
}

.fa.fa-angle-right {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need both selectors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably doesn't need the .fa selector, I will remove.

@bjacobel bjacobel force-pushed the bjacobel/breadcrumbs branch from dc4bc7b to 9c597ec Compare June 24, 2016 17:32
@bjacobel
Copy link
Contributor Author

@AlasdairSwan Pushed an update that bumps the UITK dependency to 1.4.1, removing that unnecessary sr-is-focusable.

@AlasdairSwan
Copy link
Contributor

Looks good @bjacobel 👍

@bjacobel bjacobel merged commit a200904 into master Jun 24, 2016
@bjacobel bjacobel deleted the bjacobel/breadcrumbs branch June 24, 2016 17:56
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