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

Make amp-carousel always show controls in emails #25714

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

fstanis
Copy link
Contributor

@fstanis fstanis commented Nov 21, 2019

Fixes #25713

@sparhami
Copy link

I feel like this should be done in a generic way, at some level asking the viewer if they support swipe. There are other components that may need the same treatment. Another option is to enforce the usage of the controls attribute via the validator, with a separate spec that requires it for the email format.

/cc @dvoytenko

@fstanis
Copy link
Contributor Author

fstanis commented Nov 22, 2019

I feel like this should be done in a generic way, at some level asking the viewer if they support swipe.

I like this. Maybe even a doc-level parameter that overrides isMouseDetected? For example, https://amp.example/page#mouse=1 forces that function to return true while https://amp.example/page#mouse=0 forces it to be false?

This not just solves the issue for email (since email clients control the viewer and can choose to override this), but also helps test mouse-related functionality for developers.

Another option is to enforce the usage of the controls attribute via the validator, with a separate spec that requires it for the email format.

This was my first idea, but it breaks existing emails and templates and puts needless burden on developers. I'd rather we solve this on runtime or viewer level if at all possible.

@dvoytenko
Copy link
Contributor

I feel like this should be done in a generic way, at some level asking the viewer if they support swipe.

I'm pretty sure that has already been done at some point before. For instance, don't we always show carousel controls on desktop?

@sparhami
Copy link

sparhami commented Dec 4, 2019

I feel like this should be done in a generic way, at some level asking the viewer if they support swipe.

I'm pretty sure that has already been done at some point before. For instance, don't we always show carousel controls on desktop?

Carousel 0.1 uses .amp-mode-mouse, but I'm not sure whether adding this class in emails for mobile devices would be the right thing to do. I think we should treat it as @media (pointer: fine). Carousel 0.2 shows the controls until the user changes slides specifically with a touch.

It seems like carousel is the only AMP-internal thing using .amp-mode-mouse, but it seems like sites use it in their CSS. Since email is new, it may be safe to add it in the email format, but I think having a separate mechanism would be preferable.

@fstanis
Copy link
Contributor Author

fstanis commented Dec 6, 2019

For instance, don't we always show carousel controls on desktop?

They don't show on my Linux laptop that has a touchscreen.

but I'm not sure whether adding this class in emails for mobile devices would be the right thing to do

This isn't sufficient I think - I just checked, my laptop has both amp-mode-touch amp-mode-mouse and the controls don't show.

Now that Gmail is rolling out mobile support, I'd merge this PR as a temporary solution for #25713. I'm happy to revert it when we have a better way to do it, e.g. via a viewer capability, but I'd like to avoid carousels breaking on mobile as more users start getting AMP emails there and we haven't communicated proactively enough about this feature.

@fstanis fstanis merged commit e45c22f into ampproject:master Feb 25, 2020
@fstanis fstanis deleted the controls branch February 25, 2020 21:40
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Feb 27, 2020
* master: (54 commits)
  inabox-resources: Minor test improvement (ampproject#26916)
  DocInfo: replace metaTags with viewport in API (ampproject#26687)
  🐛 SwG now uses AMP sendBeacon interface (ampproject#26970)
  🏗 Allow array destructuring on preact hooks (ampproject#26901)
  Gulp Dep Check: fail on unused entries (ampproject#26981)
  Update no-import lint rule to forbid sub-paths (ampproject#26531)
  🐛 amp-ad type blade - fix bladeOnLoad callback (ampproject#26627)
  📖 Clarify when max-age is required (ampproject#26956)
  ♻️ Consolidate players as .i-amphtml-media-component (ampproject#26967)
  Add Preact Enzyme tests (ampproject#26529)
  Fixes `update_tests` flag on `gulp validator` (ampproject#26965)
  📦 Update dependency google-closure-library to v20200224 (ampproject#26986)
  🏗 Transform aliased configured components (ampproject#26541)
  ✨ InaboxResources: Observe intersections for some elements' viewportCallbacks (ampproject#26942)
  variable substitutions: Support allowlist lookup in AmpDocShadow (ampproject#26731)
  cl/297197875 Revision bump for ampproject#26877 (ampproject#26982)
  Json fix (ampproject#26971)
  📦 Update dependency mocha to v7.1.0 (ampproject#26976)
  Add documentation for amp-access-scroll (ampproject#26782)
  make controls always shown in amp for email (ampproject#25714)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make amp-carousel[controls] the default behavior in AMP for Email
7 participants