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

Slideshow: Update mobile styles #12204

Merged
merged 2 commits into from
May 10, 2019
Merged

Conversation

thomasguillot
Copy link
Contributor

Optimise the mobile styles of the Slideshow Block and make sure we don't display unnecessary elements (prev/next arrow) on small screens.

Changes proposed in this Pull Request:

  • Only display prev/next arrows when viewport is >= 600px
  • Make sure captions fit properly within the slide and don't overflow the play/pause button.
  • Make sure slides on focus (touch slide) don't display an outline

Testing instructions:

  • Create a post with a Slideshow block
  • Add a few images with a very very long caption
  • How does the block look like?
  • Enable autoplay
  • How does the block look like?
  • Resize your screen (or check the side on mobile), do you see the arrows?
  • On on your desktop, do you see the arrows?

Before:
Screenshot 2019-04-30 at 12 23 27

After:
Screenshot 2019-04-30 at 12 23 41

Proposed changelog entry for your changes:

  • Slideshow Block: Depending on viewport, display prev/next arrows
  • Slideshow Block: Remove outline when focussing on the block

* Hide next/prev and mobile + add max-height to caption
* Update caption max-height if autoplay enabled
* Make sure slides on focus (touch slide) don't display an outline
@thomasguillot thomasguillot added [Type] Good First Bug [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Slideshow labels Apr 30, 2019
@thomasguillot thomasguillot requested a review from a team as a code owner April 30, 2019 11:27
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello thomasguillot! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D27615-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Apr 30, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: May 7, 2019.
Scheduled code freeze: April 30, 2019

Generated by 🚫 dangerJS against c7b0f22

@matticbot
Copy link
Contributor

thomasguillot, Your synced wpcom patch D27615-code has been updated.

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Works well, tested with mobile and Firefox on desktop. Tested with and without "autoplay" on.

image

Does this cause any accessibility issues potentially now that next/prev are hidden on small screens? Suppose folks using keyboard to navigate are on bigger screens anyway?

@simison simison added this to the 7.4 milestone Apr 30, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 30, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works well for me. 👍

@scruffian
Copy link
Member

@davidakennedy for the accessibility question...

@jeffersonrabb
Copy link
Contributor

Does this cause any accessibility issues potentially now that next/prev are hidden on small screens?

On mobile, swiping would be the natural form of interaction anyway. The arrows are primarily meant as shortcut for environments where pointing devices are the norm—even though you can swipe with the mouse it's unusual behavior. Because of this, I don't think this qualifies as an accessibility concern, but @davidakennedy will have a more sophisticated POV.

@kraftbj
Copy link
Contributor

kraftbj commented May 2, 2019

Holding pending the question. Marking as "needs review" for now.

@kraftbj kraftbj added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Focus] Accessibility Improving usability for all users (a11y) and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 2, 2019
@davidakennedy
Copy link

Does this cause any accessibility issues potentially now that next/prev are hidden on small screens?

I think these changes are okay. From an accessibility perspective, they still have the pagination buttons there to help them navigate around. Although, I like the arrow buttons better because they have a larger hit target.

Some customers may have no vision or low vision, so seeing what's there (to be able to swipe it) could be a problem. So screen reader users need some controls to let them know what's there, and to navigate.

This does bring up a larger issue, we're assuming all screen sixes less than 600px are touch screen. Is that a safe assumption? I think probably – I'm just raising it.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 9, 2019
@jeherve jeherve merged commit ae32198 into master May 10, 2019
@jeherve jeherve deleted the update/slideshow-block-mobile-styles branch May 10, 2019 09:14
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 10, 2019
jeherve added a commit that referenced this pull request May 17, 2019
jeherve added a commit that referenced this pull request May 23, 2019
jeherve added a commit that referenced this pull request May 27, 2019
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Slideshow [Focus] Accessibility Improving usability for all users (a11y) [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Good First Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants