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

[Master feature] amp-carousel v2 #16509

Open
ericlindley-g opened this Issue Jul 2, 2018 · 4 comments

Comments

@ericlindley-g
Copy link
Collaborator

ericlindley-g commented Jul 2, 2018

Re-factor and re-launch a new version of amp-carousel to address a number of user & developer needs, including dynamic & responsive content, UX updates, and using amp-carousel outside of the context of valid AMP

Fix some low-hanging fruit for amp-carousel UX

  • Better advance affordance visual design (improved arrow icon)
  • better "hint" affordance (currently appears and disappears without optimal timing—there could be a better pattern)

Support dynamic and responsive content

  • Support dynamic content (updates via amp-list & amp-bind to fetch and display slides on initial page load, as well as after amp-bind mutations)
  • Support responsive content (change number of slides displayed per view based on viewport width)

Make amp-carousel function well when outside of the context of a valid AMP document

More TBD — the above description includes requirements based on initial discussions
/cc @nainar for edits/updates as the scope is developed & refined

(Note: this issue and the initial comments applied just to "dynamic and responsive" content in amp-carousel)

@jaygray0919

This comment has been minimized.

Copy link

jaygray0919 commented Jul 2, 2018

@ericlindley-g Eric, here's a preliminary attempt to do some of what you describe: https://afdsi.org/test/amp_list_carousel/. We had help from @jpettitt John Pettitt and @sebastianbenz Sebastian Benz here. How can we improve this approach? What does it fail to do that you want to do? We had to 'hard-wire' your second bullet and would like to fit a series of images - perhaps with different widths - into a viewport

@ericlindley-g

This comment has been minimized.

Copy link
Collaborator Author

ericlindley-g commented Jul 2, 2018

Looks great! (though on mobile I'm not seeing any carousel content)

/cc @aghassemi for thoughts/feedback on the approach.

It's hard to tell what's missing given the issue seeing what loads on mobile, but my bet is that by adding straightforward declarative markup to the components themselves we could make this process a lot easier.

@jpettitt

This comment has been minimized.

Copy link
Collaborator

jpettitt commented Jul 2, 2018

What I'd love to see is something that looks like the snippet below which feels like "obvious" way I'd try to do it as a page developer after reading the amp-list doc.

<amp-list template="slides" ...></amp-list>
<amp-carousel ...>
  <template id="slides">
    <amp-img src="{{slideLocation}}" ...>
  <template>
</amp-carousel>

@ericlindley-g ericlindley-g changed the title [Master feature] Support dynamic & responsive content in amp-carousel [Master feature] amp-carousel v2 Oct 12, 2018

@ericlindley-g ericlindley-g moved this from Next Up to In Development in AMP HTML Project Roadmap Oct 12, 2018

@aghassemi aghassemi referenced this issue Jan 9, 2019

Open

[Roadmap] 2019 - Q1 #2

5 of 11 tasks complete

@nainar nainar added this to 2019 Q1 in UI - Master Issues Jan 17, 2019

@ampprojectbot

This comment has been minimized.

Copy link
Collaborator

ampprojectbot commented Jan 24, 2019

This issue hasn't been updated in awhile. @nainar Do you have any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.