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

refactor(carousel): merge amp-slides into amp-carousel component #301

Merged
merged 5 commits into from
Sep 28, 2015
Merged

refactor(carousel): merge amp-slides into amp-carousel component #301

merged 5 commits into from
Sep 28, 2015

Conversation

erwinmombay
Copy link
Member

No description provided.

}
});
constructor(element) {
if (element.hasAttribute('type') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked around and I feel like it's fine. Unless someone finds anything else to the contrary, let's go with it.

@erwinmombay erwinmombay self-assigned this Sep 27, 2015
@erwinmombay erwinmombay changed the title [WIP] merge carousel and slide codebase and delegate to the intended class for implementation. refactor(carousel): merge amp-slides into amp-carousel component Sep 28, 2015
@erwinmombay
Copy link
Member Author

@dvoytenko PTAL

export class BaseCarousel extends AMP.BaseElement {

addButtons() {
this.prevButton_ = document.createElement('button');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

got it.

@dvoytenko
Copy link
Contributor

Looks great, but a little more needed:

  1. Update everything.amp.html
  2. Update amp-carousel.md with new type attribute. Please explicitly describe that type=carousel currently does not support layout=responsive
  3. Update amp-slides.md to mark it as deprecated.

Thanks!

@erwinmombay
Copy link
Member Author

@dvoytenko PTAL

@erwinmombay
Copy link
Member Author

updating the docs.

/** @override */
buildCallback() {
this.buildCarousel();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

addButtons should go here, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

will do, does it make more sense to call it buildButtons ?

Copy link
Member Author

Choose a reason for hiding this comment

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

done. and renamed to buildButtons

@dvoytenko
Copy link
Contributor

LGTM once docs comments are addressed.

@erwinmombay
Copy link
Member Author

@dvoytenko updated docs.

  • added deprecation warning on amp-slides.md and linked to amp-carousel.md.
  • added type attribute and its options to amp-carousel.md
  • added comment that amp-carousel type=carousel does not currently support layout=responsive

@erwinmombay
Copy link
Member Author

just waiting on CI to pass. will merge asap afterwards

erwinmombay added a commit that referenced this pull request Sep 28, 2015
refactor(carousel): merge amp-slides into amp-carousel component
@erwinmombay erwinmombay merged commit 7615c13 into ampproject:master Sep 28, 2015
@erwinmombay
Copy link
Member Author

@dvoytenko merged. please rebase when you can

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.

2 participants