amp.dev icon indicating copy to clipboard operation
amp.dev copied to clipboard

The latest news carousel's "previous" button is pointing forward

Open DerekNonGeneric opened this issue 3 years ago • 4 comments

🐛 Bug Report

Current behavior

A clear and concise description of the bug you found in amp.dev.

Please also provide:

  • Page link: https://amp.dev/
  • (Optional) Screenshots/videos:

image

Expected behavior/code

A clear and concise description of what you expected to happen.

On the home page, the latest news carousel's "previous" button is pointing forward (to the right) instead of backward (to the left) as can be seen both in the screenshot provided.

Reproduction procedure

Please provide the steps to reproduce the problem:

  1. Visit https://amp.dev
  2. View the "Latest news" section's carousel buttons
  3. Click the "next" button
  4. Notice that "previous" is still pointing in the wrong direction
  • What browser are you using? Mozilla Firefox Developer Edition

  • What O/S are you using? Windows Server 2019

  • [x] I would like to work on this issue and submit a pull request.

/cc @CrystalOnScript

DerekNonGeneric avatar Sep 07 '21 10:09 DerekNonGeneric

Hi @DerekNonGeneric, this should have been fixed in #6109, but it hasn't made its way to production yet. Can you check locally with the latest version if the bug still occurs?

lluerich avatar Sep 08 '21 07:09 lluerich

I just built the default branch of this repo and the result (as can be seen below) shows that you all ended up changing your minds and decided to simply use the amp-carousel component instead of the custom thing that is currently in production.

image


So, the arrows are going to have to remain within the component, which seems like the reason why the more custom alternative was developed in previous iterations.

Feel free to close this issue whenever, but the issue still remains in production from what I can see.

DerekNonGeneric avatar Sep 08 '21 18:09 DerekNonGeneric

Please use http://localhost:8080/index-2021/. The screenshot you posted shows a different version of the homepage.

lluerich avatar Sep 09 '21 07:09 lluerich

Thanks @lluerich, this new carousel seems to not be mobile friendly. Once you resize the viewport to a mobile device, the tap targets are no longer accessible via mobile devices. As you can see below, the user will only have 10px to tap (and there isn't enough spacing between these elements).

image

The relevant article describing the problem and remediation can be found at https://web.dev/accessible-tap-targets/.

If nobody else is working on this, I can propose a solution in the form of a PR or we can discuss here whether arrows are desirable on mobile or if the dots are supposed to be the only way to navigate the carousel items.

DerekNonGeneric avatar Sep 09 '21 18:09 DerekNonGeneric