Add breadcrumbs JavaScript in preparation for new Pattern Library component #72
Conversation
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "edx-ui-toolkit", | |||
| "version": "1.1.0", | |||
| "version": "1.2.0", | |||
There was a problem hiding this comment.
Nit: we bump the version in a release PR so don't update it here yet.
There was a problem hiding this comment.
See here for details: https://openedx.atlassian.net/wiki/display/FEDX/UI+Toolkit+release+process
|
@bjacobel This is looking great. I have a number of minor comments so let me know when you're ready for a second review. When you've added documentation you should use |
| initialize: function (options) { | ||
| this.template = _.template(breadcrumbsTemplate); | ||
| this.listenTo(this.model, 'change', this.render); | ||
| this.render(); |
There was a problem hiding this comment.
I prefer that views do not call render in their initialize methods. There are a number of reasons for this:
- it makes it hard to control when the view is rendered
- it means that subclasses will render before their own initialization has completed
- it will render twice if you create a view, configure it, add children etc
- it doesn't allow for a component to be rendered server-side
|
Ready for re-review. |
| ;(function (define) { | ||
| 'use strict'; | ||
| define(['backbone', 'underscore', 'text!./breadcrumbs.underscore'], | ||
| function (Backbone, _, breadcrumbsTemplate) { | ||
| define(['backbone', '../utils/html-utils.js', 'text!./breadcrumbs.underscore'], |
There was a problem hiding this comment.
I had trouble consuming the UI Toolkit in edx-platform when using relative paths. I think you might need to use a full path like this:
https://github.com/edx/edx-ui-toolkit/blob/master/src/js/utils/html-utils.js#L14
|
👍 once you fix the import to not use a relative path. |
|
@andy-armstrong Sorry, which import? |
| */ | ||
| ;(function (define) { | ||
| 'use strict'; | ||
| define(['backbone', '../utils/html-utils.js', 'text!./breadcrumbs.underscore'], |
There was a problem hiding this comment.
@bjacobel I don't know where my comment went, but I had trouble using .. paths when consuming the UI Toolkit in edx-platform. I think this needs to be edx-ui-toolkit/js/utils/html-utils.js. You can see in html-utils itself that we do this to access string-utils.
There was a problem hiding this comment.
@bjacobel This is what I meant by import, to be clear.
There was a problem hiding this comment.
Sounds good. Should be fine in the tests, though, right?
bjacobel
force-pushed
the
bjacobel/breadcrumbs
branch
from
4aa42cd to
32cbcba
Compare
|
tagging @clrux for a11y review here, thanks Chris! |
|
From code alone, this looks fine. Is there a way to test? |
|
@clrux There's an example of it in the preview docs site I built for the UXPL PR, although it currently doesn't point to the latest version of this code - I'll update the preview site there once I merge this and ship a new release to NPM. |
bjacobel
force-pushed
the
bjacobel/breadcrumbs
branch
3 times, most recently
from
16b5487 to
08a1945
Compare
|
Updated with more a11y fixes with help from @clrux. Chris, you want to give a final thumbs? |
bjacobel
force-pushed
the
bjacobel/breadcrumbs
branch
from
08a1945 to
f24d518
Compare
|
Just get the styling fixed up. Markup and feedback sound OK! 👍 |
Refs TNL-4599
Adds backbone view, model and test for a breadcrumb component soon to be added to the UXPL.
UXPL PR is here.
Testing Checklist
Non-testing Checklist
Post-review
Reviewers
If you've been tagged for review, please check your corresponding box once you've given the 👍.
We'll also automatically suggest reviewers for this PR based on the code it touches.