refactor(carousel): merge amp-slides into amp-carousel component #301
Conversation
| } | ||
| }); | ||
| constructor(element) { | ||
| if (element.hasAttribute('type') && |
There was a problem hiding this comment.
I know this is still in progress. But are you returning a new instance from a constructor of another class? I always wondered how portable that "feature" is between all the browsers.
There was a problem hiding this comment.
yep, that's what i'm doing. hmm i've never read/heard anything on that not being compatible across the board. I'll research a bit more in case there is any concern. thanks.
/cc @cramforce
There was a problem hiding this comment.
I looked around and I feel like it's fine. Unless someone finds anything else to the contrary, let's go with it.
erwinmombay
changed the title
|
@dvoytenko PTAL |
| export class BaseCarousel extends AMP.BaseElement { | ||
|
|
||
| addButtons() { | ||
| this.prevButton_ = document.createElement('button'); |
There was a problem hiding this comment.
We will keep button and textContent for this PR, but we will need to update it asap in the next one. I will file the details in the bug #324.
|
Looks great, but a little more needed:
Thanks! |
|
@dvoytenko PTAL |
|
updating the docs. |
| /** @override */ | ||
| buildCallback() { | ||
| this.buildCarousel(); | ||
| } |
There was a problem hiding this comment.
addButtons should go here, right?
There was a problem hiding this comment.
will do, does it make more sense to call it buildButtons ?
There was a problem hiding this comment.
done. and renamed to buildButtons
|
LGTM once docs comments are addressed. |
|
@dvoytenko updated docs.
|
|
just waiting on CI to pass. will merge asap afterwards |
refactor(carousel): merge amp-slides into amp-carousel component
|
@dvoytenko merged. please rebase when you can |
No description provided.