FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(page): skip to content should point to primary content container by seanforyou23 · Pull Request #2519 · patternfly/patternfly-react · GitHub

fix(page): skip to content should point to primary content container - #2519

Merged
redallen merged 5 commits into
patternfly:masterfrom
seanforyou23:navigation-enhancements
Jul 26, 2019
Merged

fix(page): skip to content should point to primary content container#2519
redallen merged 5 commits into
patternfly:masterfrom
seanforyou23:navigation-enhancements

Conversation

seanforyou23 commented Jul 15, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

What: Currently, skip to content link sends users to an in-page anchor that sits alongside the main page content container. This generally has the desired effect for sighted keyboard users, however, for screen reader users (SRU) this basically leads to a dead end. It's a dead-end in that the target anchor doesn't contain the page content, so nothing further is announced after the element has been focused.

I think what would be better is if we allow the main content container to carry an id, which can be used as the target of the SkipToContent component. Doing this along with supplying a tabindex="-1" for the main content area gives us a solid hook for informing SRU of general route changes and informing them of what content is now available.

For sighted users, I don't think this will cause any difference in behavior (we may want to add a style like outline: none to the primary page container, although I don't recommend this.

Additional issues: #2591

These changes are driven by a technique suggested in https://dequeuniversity.com where in response to dynamic content changes (like navigating to a new "Page" in a SPA) we want to send focus to the new content so that it is announced and SRU receive some feedback about the new content.

matthewcarleton left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM! We should also ensure this gets updated in core and documented on the site.

Copy link
Copy Markdown
Collaborator

PatternFly-React preview: https://patternfly-react-pr-2519.surge.sh

jschuler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good, can you also update the page demos

redallen previously approved these changes Jul 23, 2019
jgiardino previously approved these changes Jul 25, 2019

jgiardino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is awesome! Thanks for all your work researching this piece and contributing it back.

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks great so far @seanforyou23 do you mind opening a React issue for tracking purposes.

I had one question. If mainContainerId is not set by consumer, does this work as expected? Maybe we need to add in a test for that?

seanforyou23 dismissed stale reviews from jgiardino and redallen via 15b8282 July 25, 2019 17:48

Copy link
Copy Markdown
Contributor Author

@tlabaj good catch, I updated the tests to reflect what we can expect in the current state. If users don't supply a mainContainerId then things will not work as expected. I'd like to make it a required prop at the next available opportunity for breaking changes.

Copy link
Copy Markdown
Contributor Author

I also fixed this error from the Page test

tlabaj commented Jul 25, 2019

Copy link
Copy Markdown
Contributor

@seanforyou23 let's open a follow up issue to make that required so we can pull that into the next breaking change release.

tlabaj commented Jul 26, 2019

Copy link
Copy Markdown
Contributor

@seanforyou23 I opened issue #2597 for the breaking change.
Can you open an issue in core to make the updates that @matthewcarleton mentioned please.

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

redallen dismissed jschuler’s stale review July 26, 2019 20:00

Demos updated.

redallen merged commit 1f709bd into patternfly:master Jul 26, 2019

Copy link
Copy Markdown
Collaborator

Your changes have been released in:

  • @patternfly/react-core@3.77.2
  • @patternfly/react-docs@4.9.8
  • @patternfly/react-inline-edit-extension@2.9.54
  • demo-app-ts@2.13.2
  • @patternfly/react-integration@2.13.1
  • @patternfly/react-table@2.15.2
  • @patternfly/react-topology@2.7.3
  • @patternfly/react-virtualized-extension@1.1.87

Thanks for your contribution! 🎉

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants


Back | FazBrowse Home | New Git URL