Skip to content

Slideshow: Update mobile styles#12204

Merged
jeherve merged 2 commits intomasterfrom
update/slideshow-block-mobile-styles
May 10, 2019
Merged

Slideshow: Update mobile styles#12204
jeherve merged 2 commits intomasterfrom
update/slideshow-block-mobile-styles

Conversation

@thomasguillot
Copy link
Copy Markdown
Contributor

@thomasguillot thomasguillot commented Apr 30, 2019

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

@thomasguillot
Slideshow: Update mobile styles
c23a840 
* 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 This PR is ready for review. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Slideshow labels Apr 30, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Apr 30, 2019

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
Copy Markdown
Collaborator

jetpackbot commented Apr 30, 2019 •
edited
Loading

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

simison
simison reviewed Apr 30, 2019
View reviewed changes
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Apr 30, 2019

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

simison
simison approved these changes Apr 30, 2019
View reviewed changes
Copy link
Copy Markdown
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 This PR is ready for review. labels Apr 30, 2019
jeherve
jeherve approved these changes Apr 30, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

scruffian commented Apr 30, 2019

@davidakennedy for the accessibility question...

@jeffersonrabb
Copy link
Copy Markdown
Contributor

jeffersonrabb commented Apr 30, 2019

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
Copy Markdown
Contributor

kraftbj commented May 2, 2019

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

@kraftbj kraftbj added [Status] Needs Review This PR is ready for review. [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
Copy Markdown

davidakennedy commented May 8, 2019

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 This PR is ready for review. 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
Changelog: add #12204
ddfab2f
jeherve added a commit that referenced this pull request May 23, 2019
@jeherve
Changelog: add #12204
59e22bb
jeherve added a commit that referenced this pull request May 27, 2019
@kraftbj @jeherve
7.4 Changelog (#12304)
0fb4436 
* 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

Reviewers

@simison simison simison approved these changes

@jeherve jeherve jeherve approved these changes

@scruffian scruffian Awaiting requested review from scruffian

@jeffersonrabb jeffersonrabb Awaiting requested review from jeffersonrabb

Assignees

No one assigned

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

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

9 participants

@thomasguillot @matticbot @jetpackbot @scruffian @jeffersonrabb @kraftbj @davidakennedy @simison @jeherve