a11y mobile toc #7850
a11y mobile toc #7850
Conversation
There was a problem hiding this comment.
Just tested the staging site and noticed a couple things:
- When I navigate to a page that has table of contents, the left side nav no longer scrolls for me (tested on this page https://a11y-mobile-toc.d1egzztxsxq9xz.amplifyapp.com/react/start/manual-installation/)
- When I tab past the last item in the table of contents, the focus seems to get lost. It looks like there is a duplicated table of contents in the DOM?
| )} | ||
| </div> | ||
| </View> | ||
| {showTOC ? <TableOfContents headers={tocHeadings} /> : null} |
There was a problem hiding this comment.
For the table of contents can we find a way to keep it where it is currently in the DOM? Putting it in the header makes it more difficult for keyboard navigators to get to on desktop, where as if it's still in the <main> element they can at least use the skip to main link
There was a problem hiding this comment.
Thanks for the updates @katiegoines I think we're getting closer; probably there should have been a little more direction scrutinized on this one on my part. Left some comments about a broken functionality and some other suggestions/questions inline.
Additionally, some layout questions (and feel free to pushback on any of this, or maybe we can follow up at another time, these aren't particularly crucial):
- Is it too awkward with the layout/css to only have one table of contents markup in the DOM? If probably makes the existing slide out menu CSS a little less re-usable if we did that but was wondering your thoughts on it.
- Would it be difficult to make the "mobile" table of contents drop in sooner? The table of contents flips to below the content at the 1360px mark which differs from the mobile menu (that moves to the mobile version at 1000px).
And a styling suggestion:
- Can we pretty this up? Suggest dropping the border and margin, and increasing the padding when the table of contents is in this slide out drawer.
Before
After
| )} | ||
| </div> | ||
| </View> | ||
| {showTOC ? <div className=""></div> : null} |
There was a problem hiding this comment.
Is this needed anymore?
| {showTOC ? <div className=""></div> : null} |
| } else { | ||
| toggleMenuOpen(false); | ||
| // For keyboard navigators, move focus back to menu button in header | ||
| menuButtonRef?.current?.focus(); |
There was a problem hiding this comment.
Looks like this isn't working correctly with these changes. When I test keyboard closing the menu, it now focuses the table of contents toggle instead of the menu toggle.
| ref={tocButtonRef} | ||
| className="search-menu-toggle mobile-toggle" | ||
| > | ||
| <IconMenu aria-hidden="true" /> |
There was a problem hiding this comment.
Nit: We can use the IconTOC here.
Pull request was closed
katieklein
force-pushed
the
a11y-mobile-toc
branch
from
b7ed01e to
63616a6
Compare
|
|
Description of changes:
Implemented mobile TOC that functions like the mobile menu.
Before: on mobile, TOC content was at bottom
After: TOC content is in collapsible menu that functions like the main nav
Staging: https://a11y-mobile-toc.d1egzztxsxq9xz.amplifyapp.com/react/start/quickstart/#1-create-the-repository
Related GitHub issue #, if available:
Instructions
If this PR should not be merged upon approval for any reason, please submit as a DRAFT
Which product(s) are affected by this PR (if applicable)?
Which platform(s) are affected by this PR (if applicable)?
Please add the product(s)/platform(s) affected to the PR title
Checks
Does this PR conform to the styleguide?
Does this PR include filetypes other than markdown or images? Please add or update unit tests accordingly.
Are any files being deleted with this PR? If so, have the needed redirects been created?
Are all links in MDX files using the MDX link syntax rather than HTML link syntax?
ref: MDX:
[link](https://docs.amplify.aws/)HTML:
<a href="https://docs.amplify.aws/">link</a>When this PR is ready to merge, please check the box below
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.