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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Feb 24, 2020
Merged

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

merged 6 commits into from
Feb 24, 2020

Conversation

wigglemuff
Copy link
Contributor

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.

@wigglemuff wigglemuff added [Type] 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 To request a review from Crew. Label will be renamed soon. Good For Community labels Oct 13, 2019
@wigglemuff wigglemuff requested a review from a team as a code owner October 13, 2019 07:48
@wigglemuff wigglemuff self-assigned this Oct 13, 2019
@matticbot
Copy link
Contributor

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

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
modules/shortcodes/js/slideshow-shortcode.js Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 14, 2019
@CarlyGerard
Copy link

@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
Contributor

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!

@wigglemuff
Copy link
Contributor Author

@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
Contributor

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

@matticbot
Copy link
Contributor

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

@wigglemuff wigglemuff added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 2, 2020
@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
modules/shortcodes/slideshow.php Outdated Show resolved Hide resolved
@matticbot
Copy link
Contributor

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

jeherve
jeherve requested changes Feb 19, 2020
View reviewed changes
modules/shortcodes/slideshow.php Outdated Show resolved Hide resolved
@matticbot
Copy link
Contributor

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

jeherve
jeherve requested changes Feb 19, 2020
View reviewed changes
modules/shortcodes/slideshow.php Outdated Show resolved Hide resolved
modules/shortcodes/js/slideshow-shortcode.js Outdated Show resolved Hide resolved
@matticbot
Copy link
Contributor

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

@wigglemuff
Copy link
Contributor Author

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

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

@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 21, 2020
jeherve
jeherve approved these changes Feb 21, 2020
View reviewed changes
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 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 To request a review from Crew. Label will be renamed soon. labels Feb 24, 2020
kraftbj
kraftbj approved these changes Feb 24, 2020
View reviewed changes
Copy link
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
Contributor

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
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

@wigglemuff wigglemuff

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

Successfully merging this pull request may close these issues.
8 participants
@wigglemuff @matticbot @jetpackbot @CarlyGerard @coleshaw @GaryJones @jeherve @kraftbj