Skip to content

Slideshow Shortcode: Add labels to navigation buttons for accessibility#13739

Merged
kraftbj merged 6 commits intomasterfrom
fix/slideshow-aria
Feb 24, 2020
Merged

Slideshow Shortcode: Add labels to navigation buttons for accessibility#13739
kraftbj merged 6 commits intomasterfrom
fix/slideshow-aria

Conversation

@rakmob42
Copy link
Copy Markdown

@rakmob42 rakmob42 commented Oct 13, 2019

Changes proposed in this Pull Request:

Testing instructions:

  • Enable these modules: 1. Tiled Galleries 2. Shortcodes Embeds
  • Install Classic Editor plugin and activate it.
  • Add a gallery slideshow in a new post
  • Inspect the code and you'll find aria-label and role attributes for slideshow controls (check the screenshot below)

Screen Shot on 2019-10-13 at 13:14:05

Proposed changelog entry for your changes:

  • Accessibility labels for Slideshow shortcode

Other notes:

I have made changes to the JS file and not the PHP file because the controls are rendered via JS. Thus it is much simpler to make the change this way.

@rakmob42 rakmob42 added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Shortcodes / Embeds [Feature] Extra Sidebar Widgets [Focus] Accessibility Improving usability for all users (a11y) [Status] Needs Review This PR is ready for review. Good For Community labels Oct 13, 2019
@rakmob42 rakmob42 requested a review from a team October 13, 2019 07:48
@rakmob42 rakmob42 self-assigned this Oct 13, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Oct 13, 2019

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

@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Oct 13, 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: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against c34824c

jeherve
jeherve previously requested changes Oct 14, 2019
View reviewed changes
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Oct 14, 2019
@CarlyGerard
Copy link
Copy Markdown

CarlyGerard commented Nov 15, 2019

@codestor4 @jeherve is there anything I can do to move this PR along? I don't want to step on anyone's work, but having this fix would help our site owners out. Thank you!

@coleshaw
Copy link
Copy Markdown
Contributor

coleshaw commented Jan 31, 2020

Hi! I'm trying to get started with WP and would be interested in helping push this across the finish line .... @codestor4 , @jeherve , is there anything I can do? I've made the suggested changes locally on top of the original PR, but not sure what the proper process or etiquette would be for this. Thanks!

@rakmob42
Copy link
Copy Markdown
Author

rakmob42 commented Feb 1, 2020

@coleshaw Thanks for the ping. I'm going to try and get to this before Monday but if I don't, feel free to take this one. The next steps on submitting a patch are available here - https://github.com/Automattic/jetpack/blob/master/docs/CONTRIBUTING.md#write-and-submit-a-patch

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 2, 2020

codestor4, Your synced wpcom patch D33966-code has been updated.

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 2, 2020

codestor4, Your synced wpcom patch D33966-code has been updated.

@rakmob42 rakmob42 added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 2, 2020
@rakmob42 rakmob42 requested a review from jeherve February 2, 2020 08:29
@jeherve jeherve added this to the 8.3 milestone Feb 3, 2020
@kraftbj kraftbj dismissed jeherve’s stale review February 12, 2020 17:15

Issues addressed.

@kraftbj kraftbj requested review from kraftbj and removed request for jeherve February 12, 2020 17:31
kraftbj
kraftbj previously requested changes Feb 12, 2020
View reviewed changes
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 19, 2020

codestor4, Your synced wpcom patch D33966-code has been updated.

@rakmob42 rakmob42 requested a review from kraftbj February 19, 2020 17:25
jeherve
jeherve requested changes Feb 19, 2020
View reviewed changes
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 19, 2020

codestor4, Your synced wpcom patch D33966-code has been updated.

@rakmob42 rakmob42 requested a review from jeherve February 19, 2020 19:08
jeherve
jeherve requested changes Feb 19, 2020
View reviewed changes
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 20, 2020

codestor4, Your synced wpcom patch D33966-code has been updated.

@rakmob42
Copy link
Copy Markdown
Author

rakmob42 commented Feb 21, 2020

Requested changes are complete. The final result works as expected:

Screen Shot on 2020-02-22 at 01:00:47

@rakmob42 rakmob42 requested a review from jeherve February 21, 2020 19:31
@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 21, 2020
jeherve
jeherve approved these changes Feb 21, 2020
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 too, but I'll let someone else review as well so we get more eyes on that last iteration. 👍

@jeherve jeherve dismissed kraftbj’s stale review February 24, 2020 07:17

Feedback was addressed

@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 Feb 24, 2020
kraftbj
kraftbj approved these changes Feb 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Nice. Works as intended and approach looks good to me.

@kraftbj kraftbj merged commit 8536eb1 into master Feb 24, 2020
@kraftbj kraftbj deleted the fix/slideshow-aria branch February 24, 2020 17:35
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 24, 2020
@GaryJones
Copy link
Copy Markdown
Contributor

GaryJones commented Feb 24, 2020

Definitely an improvement, thanks!

Has there been any consideration of changing

<a href="#" ... role="button">

to

<button>

?

(I can open a new Issue if not.)

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Feb 24, 2020

@GaryJones I don't believe this has been discussed, no.

You can certainly open an issue, however I should mention that the slideshow shortcode isn't really under active development right now since we now have a Slideshow block.

We can review Pull Requests if you'd like to contribute a patch though!

jeherve added a commit that referenced this pull request Feb 25, 2020
@jeherve
Changelog: add #13739
ee01e01
jeherve added a commit that referenced this pull request Feb 25, 2020
@jeherve
Changelog: 8.3 running changelog (#14642)
8797bea 
* 8.3 release: changelog

* Changelog: add #14516

* Changelog: add #14574

* Bring in changes from 8.2.1 and 8.2.2

* Update stable version

* Bring in 8.2.3 changes

* Changelog: add #14714

* Changelog: add #14639

* Changelog: add #14678

* Changelog: add #14673

* Changelog: add #14687

* Changelog: add #14704

* Changelog: add #14702

* Changelog: add #14541

* Changelog: add #14657

* Changelog: add #14622

* Changelog: add #14582

* Changelog: add #14638

* Changelog: add #14633

* Changelog: add #14571

* Changelog: add #14592

* Changelog: add #14539

* Changelog: add #14514

* Changelog: add #14643

* Changelog: add #14494

* Changelog: add #13739

* Changelog: add #14707

* Changelog: add #14736

* Changelog: add #14706

* Changelog: add #14730

* Changelog: add #14685

* Changelog: add #14727

* Changelog: add #14711

* Changelog: add #14742

* Changelog: add #14746

* Changelog: add #14725

* Changelog: add #13999

* Changelog: add #14740

* Changelog: add #14759

* Changelog: add #14703

* Changelog: add #14753

* Changelog: add #14754

* Changelog: add #14645

* Cahngelog: add #14599
@scottsweb scottsweb mentioned this pull request Jun 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kraftbj kraftbj kraftbj approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@rakmob42 rakmob42

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Feature] Shortcodes / Embeds [Focus] Accessibility Improving usability for all users (a11y) Good For Community Touches WP.com Files

Projects

None yet

Milestone

8.3

Development

Successfully merging this pull request may close these issues.

8 participants

@rakmob42 @matticbot @jetpackbot @CarlyGerard @coleshaw @GaryJones @jeherve @kraftbj