✨ ✅ Added MDC linear progress bar and tests#20
Conversation
| { | ||
| "name": "ember-material-components-web", | ||
| "version": "0.0.33", | ||
| "version": "0.0.34", |
There was a problem hiding this comment.
Let's leave the version bump out of the PR, in case we want to release multiple PRs in a single version.
There was a problem hiding this comment.
I don't understand why that would be any more desirable here than all the other times we've bumped the version number in this repo within a single PR.
There was a problem hiding this comment.
This is our first non-external PR in this repo :)
| const value = get(this, prop); | ||
| const Prop = Ember.String.capitalize(prop); | ||
| if (foundation[`is${Prop}`]() !== value) { | ||
| if (!foundation[`is${Prop}`] || foundation[`is${Prop}`]() !== value) { |
There was a problem hiding this comment.
I wonder if this is a safe assumption to make -- if you call sync('foo') and there's no isFoo() method on the foundation, there will still be a setFoo() method. Probably a fine assumption until proven wrong, but should probably be noted in the JSDoc for sync().
| //endregion | ||
|
|
||
| //region Methods | ||
| /** |
There was a problem hiding this comment.
@param for the foundation that is passed
| hasClass: (className) => get(this, 'element').classList.contains(className), | ||
| addClass: (className) => Ember.run(() => get(this, 'mdcClasses').addObject(className)), | ||
| removeClass: (className) => Ember.run(() => get(this, 'mdcClasses').addObject(className)), | ||
| getPrimaryBar: () => this.$(strings.PRIMARY_BAR_SELECTOR), |
There was a problem hiding this comment.
getPrimaryBar and getBuffer want HTMLElements, not jQuery objects. I recommend using document.querySelector for these adapter methods -- and wrapping that call in getElementProperty for safety. Example here
There was a problem hiding this comment.
Why would I wrap it in getElementProperty? That returns a property value, but this adapter method wants the HTML element like you mentioned.
There was a problem hiding this comment.
You use getElmentProperty to get access to the querySelector method of the component's element. Then you use that method to get access to the HTML Element within the component's element that the adapter wants.
There was a problem hiding this comment.
So I was able to get HTML elements here, but I run into trouble later when it tries to use .css() on the elements within the setStyle adapter method. Apparently .css() is only acceptable on jQuery objects?
There was a problem hiding this comment.
Maybe I can try to use setStyleFor instead of .css
|
|
||
| export default Ember.Component.extend(MDCComponent, { | ||
| //region Attributes | ||
| role: 'progressbar', |
There was a problem hiding this comment.
Is it desired that users of {{mdc-linear-progress}} can pass in role to change it via the HBS? If not this might be better off in //region Properties.
| //endregion | ||
|
|
||
| //region Properties | ||
| mdcLinearProgress: null, |
There was a problem hiding this comment.
I believe you have an unused property here.
| //region Methods | ||
| createFoundation() { | ||
| return new MDCLinearProgressFoundation({ | ||
| hasClass: (className) => get(this, 'element').classList.contains(className), |
There was a problem hiding this comment.
Instead of using the DOM, use get(this, 'mdcClasses').includes(className).
There was a problem hiding this comment.
I can change it here, but just to let you know there are a couple other places we'll need to make this change as well at some point - mdc-toolbar.js and mdc-menu.js.
| return new MDCLinearProgressFoundation({ | ||
| hasClass: (className) => get(this, 'element').classList.contains(className), | ||
| addClass: (className) => Ember.run(() => get(this, 'mdcClasses').addObject(className)), | ||
| removeClass: (className) => Ember.run(() => get(this, 'mdcClasses').addObject(className)), |
There was a problem hiding this comment.
You have addObject here in removeClass.
| { name: '@material/tabs', css: true, js: true }, | ||
| { name: '@material/ripple', css: true, js: true } | ||
| { name: '@material/ripple', css: true, js: true }, | ||
| { name: '@material/linear-progress', css: true, js: true} |
There was a problem hiding this comment.
Minor nitpick, but you're missing a space before the closing }.
We should install Prettier.js.
There was a problem hiding this comment.
aagh whoops, I'm normally really good about spaces... 😅
| <section class="component-doc"> | ||
| <h2 class="mdc-typography--title">Progress Bar</h2> | ||
|
|
||
| <h3 class="mdc-typography--subheading2">Linear Progress Determinate</h3> |
There was a problem hiding this comment.
It might be nice to have an ember input bound to {{myProgress}} for the demo so you can see it change.
- ✨ Implement @material/linear-progress (#20)
- ✨ Implement @material/linear-progress (#20)
2 participants
Resolves Issue #8
Material Design Linear Progress component is now available for use in Ember Material Components Web.