Amp-compare-slider: Brand New Component Skeleton #10909
Conversation
aria-labels on the 'back' and 'next' buttons of the carousel can now be changed instead of defaulting to 'next/previous item in carousel'
This reverts commit 91a75a9.
| super(element); | ||
|
|
||
| /** @private {string} */ | ||
| this.myText_ = 'hello world'; |
There was a problem hiding this comment.
remove this. Add it back if used later
| this.myText_ = 'hello world'; | ||
|
|
||
| /** @private {!Element} */ | ||
| this.container_ = this.win.document.createElement('div'); |
There was a problem hiding this comment.
Please delay document API until #buildCallback. Only initiate this.container_ in #constructor, and assign its value in #buildCallback
| </tr> | ||
| <tr> | ||
| <td width="40%"><strong>Availability</strong></td> | ||
| <td>FILL THIS IN</td> |
There was a problem hiding this comment.
Indicate that the component is under development.
|
|
||
| /** @override */ | ||
| isLayoutSupported(layout) { | ||
| return layout == Layout.RESPONSIVE; |
There was a problem hiding this comment.
What's our conclusion here? Do we only support layout responsive? to @camelburrito @ericfs
There was a problem hiding this comment.
Was this actually intended for me? I don't think I'm familiar with the context here.
There was a problem hiding this comment.
ooops sorry, meant to cc @ericlindley-g on the layout format😄
There was a problem hiding this comment.
i think we will need to support everything except container .
There was a problem hiding this comment.
:) I think we'd want to support all standard layout types (fill, fixed, fixed-height, flex-item, nodisplay, responsive), unless there's a reason not to. WDYT?
/cc @aghassemi for visibility
There was a problem hiding this comment.
chatted with Alanna offline, yes all but container
There was a problem hiding this comment.
@alannawalton Let's add a TODO here. And add support to other layout in following PR.
|
Can someone give another look at this pull request please? |
|
@alannawalton Please fix the travis build failure. Try |
| /** @override */ | ||
| buildCallback() { | ||
| this.container_ = this.win.document.createElement('div'); | ||
| this.container_.textContent = this.myText_; |
There was a problem hiding this comment.
this.myText_ is undefined. Please remove this line.
|
@alannawalton Almost there. But travis is failing, please fix the test. |
|
🎉 🎉 🎉 |
All the files associated with this component. Implementation is in a different PR .