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

Create amp-carousel 0.2 based on amp-base-carousel #23393

Merged
merged 29 commits into from Aug 8, 2019

Conversation

@sparhami
Copy link
Contributor

commented Jul 18, 2019

The main goal of this PR is to create a mostly API compatible update to <amp-carousel>, addressing numerous bugs. This is not strictly API compatible due to how the arrow buttons are positioned to allow automatic flipping in RTL.

This uses the same DOM structure as amp-carousel as far as slides and their classes are concerned for both type="slides" and type="carousel". This maintains the API of the existing carousel. This does not add support for media queries in attributes, handling updating the slides or any other features of <amp-base-carousel>.

  • Fix a bad usage of clamp
  • Rename some base carousel classes to not clash
  • Make scrollbar hiding optional, as type="carousel" does not hide the scrollbar
  • Use setAsowner for managing when to schedule layout for children
  • Use IntersectionObserver to determine when to layout/unlayout/pause/resume slides
    • This is mostly important for type="carousel", where slides can be of mixed length, and we cannot rely on the current index to tell when to actually layout adjacent slides
    • If IntersectionObserver is not supported, this currently skips setAsOwner and just relies on the runtime
  • Fix arrow button hiding when at the start/end for RTL
  • Fix bug where if you touch/wheel scrolled exactly when the scroll position was being reset, the current index would not be reported correctly

Will add some e2e tests in a subsequent PR to keep this from growing too large. Also, once we are comfortable with the state of the component, docs/examples can be updated to point to 0.2.

sparhami added 12 commits Jun 6, 2019
Initial code for amp-carousel 0.2.
- Uses amp-base-carousel's implementation, with the amp-carousel API /
styling.

@googlebot googlebot added the cla: yes label Jul 18, 2019

sparhami added 2 commits Jul 19, 2019
Add more tests.
- Fix race condition with reset window position and scroll.

@sparhami sparhami force-pushed the sparhami:carousel_0_2_on_base branch from 7201324 to 20826cd Jul 24, 2019

@sparhami sparhami requested a review from alanorozco Jul 24, 2019

@sparhami sparhami marked this pull request as ready for review Jul 24, 2019

@sparhami

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@alanorozco If you don't have the bandwidth to review this, feel free to re-assign.

@sparhami sparhami requested review from cvializ and removed request for alanorozco Jul 29, 2019

@cvializ
Copy link
Contributor

left a comment

First pass

@@ -270,7 +270,7 @@ exports.extensionBundles = [
},
{
name: 'amp-carousel',
version: '0.1',
version: ['0.1', '0.2'],
latestVersion: '0.1',

This comment has been minimized.

Copy link
@cvializ

cvializ Jul 29, 2019

Contributor

What effect does it have adding 0.2 but leaving latestVersion: '0.1'?

This comment has been minimized.

Copy link
@sparhami

sparhami Jul 29, 2019

Author Contributor

The latest version is what amp-carousel-latest will point at, which is 0.1. The background to this field is that we originally had amp-base-carousel as 0.2, but it had an incompatible API and was under experiment, which broke people relying on "latest". It was decided that the latestVersion would be locked at the existing latest version at the time of the change for all extensions, and that we would deprecate the usage so we didn't break people again in the future.

/**
* @private
*/
setupActions_() {

This comment has been minimized.

Copy link
@cvializ

cvializ Jul 29, 2019

Contributor

nit: it's unusual to have a private method before the constructor

This comment has been minimized.

Copy link
@sparhami

sparhami Jul 29, 2019

Author Contributor

Its something I am trying out and also trying it out for amp-stream-gallery, which I will send out later: https://github.com/sparhami/amphtml/blob/9634040f99658bb2651b5d3a48c58fd9c97b66a3/extensions/amp-stream-gallery/0.1/amp-stream-gallery.js#L53-L128

The thinking is that actions and attributes are the public API of the component, and usually the first thing I want to see in the file to figure out what it does. I can move this down for now, but there may be some way to make this less awkward. Any thoughts?

}

/**
*

This comment has been minimized.

Copy link
@cvializ

cvializ Jul 29, 2019

Contributor

nit: add descriptive comment.

This happens to me so often when I just want the red lines to go away 😂

This comment has been minimized.

Copy link
@sparhami

sparhami Jul 29, 2019

Author Contributor

Oops! Added comments and renamed these methods.

* }
* ```
*/
export class ChildLayoutManager {

This comment has been minimized.

Copy link
@cvializ

cvializ Jul 29, 2019

Contributor

Is this needed because Layers hasn't launched yet?

This comment has been minimized.

Copy link
@sparhami

sparhami Jul 29, 2019

Author Contributor

Yep. When layers is ready, we might either take this away, or perhaps figure out if there is some value to having a carousel specific implementation (are there cases where we know what to do better than layers, which is more general?).

I think one (not very common, but possible) case is if you are say on the first slide and want to go to the nth slide (either by clicking a pagination dot, or some programmatic action). Since we smooth scroll, layers and this code will possibly (likely) load may slides along the way.

We could be a bit smarter and say to not load any slides along the way, and only load the destination slide.

Another possible difference is that we may not want to unlayout slides (or defer it) in case a user wants to go back and forth through a carousel. Currently, I believe runtime will unlayout things that exit the extended viewport area. This code does the same for now.

This actually differs from how carousel 0.1 works, which never does unlayout on slides (though it does display: none them).

* @private
*/
setupActions_() {
this.registerAction(

This comment has been minimized.

Copy link
@cvializ

cvializ Jul 29, 2019

Contributor

Can you help me understand the difference between <amp-base-carousel> and <amp-carousel> 0.2? I see that it imports the Carousel class from amp-base-carousel but the AMP element amp-base-carousel is still around.

This comment has been minimized.

Copy link
@sparhami

sparhami Jul 29, 2019

Author Contributor

Sure. <amp-carousel> 0.2 is an almost exactly API compatible replacement for 0.1, with a minor breaking change on how arrow buttons are laid out (as far as I know, that should be the only breaking change, there could be more).

<amp-base-carousel> has a different API and way of doing things that makes the API considerably different. For example, the way that the equivalent type="carousel" is handled is quite different.

The idea here is that they can share an internal implementation, which fixes a lot of outstanding issues with amp-carousel without requiring developers to do too much work to figure out how to make <amp-base-carousel> work for their use case.

In the longer term, we think most developers will want to use the amp-stream-gallery and amp-inline-gallery components, with amp-base-carousel serving as a building block.

I did consider actually instantiating an <amp-base-carousel> internally here, but wasn't sure if that was a good idea for now.

@sparhami sparhami requested a review from cvializ Aug 6, 2019

@cvializ
cvializ approved these changes Aug 6, 2019
Copy link
Contributor

left a comment

LGTM smash that merge button

sparhami added 3 commits Aug 6, 2019
sparhami added 9 commits Aug 6, 2019

@sparhami sparhami merged commit 20c7ce7 into ampproject:master Aug 8, 2019

14 of 15 checks passed

ampproject/pr-deploy Ready to create a test site.
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
ampproject/bundle-size Δ +0.01KB | no approval necessary
Details
ampproject/tests/e2e (local) 308 tests passed
Details
ampproject/tests/integration (local) 271 tests passed
Details
ampproject/tests/integration (saucelabs) 420 tests passed
Details
ampproject/tests/integration (single-pass) 213 tests passed
Details
ampproject/tests/unit (local) 10120 tests passed
Details
ampproject/tests/unit (local-changes) 25 tests passed
Details
ampproject/tests/unit (saucelabs) 7848 tests passed
Details
cla/google All necessary CLAs are signed
codecov/patch 89.27% of diff hit (within 100% threshold of 100%)
Details
codecov/project 79.41% (+0.15%) compared to 63def21
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details
Gregable added a commit to Gregable/amphtml that referenced this pull request Aug 8, 2019
erwinmombay added a commit that referenced this pull request Aug 8, 2019
Validator rollup (#23835)
* cl/262363476 Add width and height attributes to the svg SYMBOL tag validation rules.

* cl/262384419 Revision bump for #23406

* cl/262398573 Revision bump for #23393

* cl/262415464 Add devmode validation logic.

* cl/262420139 s/amphtml-dev-mode/ampdevmode/ in our error strings.

* cl/262430869 Convert python scripts to be python3 compatible.

* cl/262440817 Revision bump for #23800

* cl/262442134 Revision bump for #23826
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
Create amp-carousel 0.2 based on amp-base-carousel (ampproject#23393)
The main goal of this PR is to create a mostly API compatible update to `<amp-carousel>`, addressing numerous bugs. This is not strictly API compatible due to how the arrow buttons are positioned to allow automatic flipping in RTL.

This uses the same DOM structure as amp-carousel as far as slides and their classes are concerned for both `type="slides"` and `type="carousel"`. This maintains the API of the existing carousel. This does not add support for media queries in attributes, handling updating the slides or any other features of `<amp-base-carousel>`.

- Fix a bad usage of `clamp`
- Rename some base carousel classes to not clash
- Make scrollbar hiding optional, as `type="carousel"` does not hide the scrollbar
- Use `setOwner` for managing when to schedule layout for children
- Use `IntersectionObserver` to determine when to layout/unlayout/pause/resume slides
  * This is mostly important for `type="carousel"`, where slides can be of mixed length, and we cannot rely on the current index to tell when to actually layout adjacent slides
  * If IntersectionObserver is not supported, this currently skips `setOwner` and just relies on the runtime
- Fix arrow button hiding when at the start/end for RTL
- Fix bug where if you touch/wheel scrolled exactly when the scroll position was being reset, the current index would not be reported correctly
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
Validator rollup (ampproject#23835)
* cl/262363476 Add width and height attributes to the svg SYMBOL tag validation rules.

* cl/262384419 Revision bump for ampproject#23406

* cl/262398573 Revision bump for ampproject#23393

* cl/262415464 Add devmode validation logic.

* cl/262420139 s/amphtml-dev-mode/ampdevmode/ in our error strings.

* cl/262430869 Convert python scripts to be python3 compatible.

* cl/262440817 Revision bump for ampproject#23800

* cl/262442134 Revision bump for ampproject#23826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.