docs(JumpLinks): add jump links with drawer demo #7520
Conversation
|
Preview: https://patternfly-react-pr-7520.surge.sh A11y report: https://patternfly-react-pr-7520-a11y.surge.sh |
|
|
||
| ### With drawer | ||
|
|
||
| This demo shows how jump links can be used in combination with a drawer. |
There was a problem hiding this comment.
Could you also call out what you used as the scrollableSelector in this case? Just for clarity since I believe that was the major question which lead to the development of these demos
There was a problem hiding this comment.
This is looking good @wise-king-sullyman! Just one comment.
packages/react-core/src/demos/examples/JumpLinks/JumpLinksWithDrawer.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Jenny <32821331+jenny-s51@users.noreply.github.com>
There was a problem hiding this comment.
This looks good @wise-king-sullyman . If it's not too much trouble, can the toggle be placed in a sticky page section so it always stays at the top when the content scrolls?
@jgiardino @srambach looks like the original request for this came from you. Is this what you were hoping to see?
|
@mcarrano does this implementation of your sticky page section suggestion look good to you? The default variant looked a bit out of place to me so I went with the light variant, but I'm happy to change it. |
|
Looks good to me. Thanks for adding that @wise-king-sullyman . |
|
|
||
| This demo shows how jump links can be used in combination with a drawer. | ||
|
|
||
| The scrollable selector passed to the jump links component is an id that was placed on the drawer content component. |
There was a problem hiding this comment.
I think it may even be incrementally more helpful if you use the markdown tics to emphasize the prop and component names in these instructions.
Maybe:
'The scrollableSelector prop passed to the jump links component should reference an id that was placed on the DrawerContent component.'
|
This looks great; definitely showing what I had hoped, how to put both a drawer and jumplinks together in a page. I did notice though that the Skip to Content link doesn't go anywhere - I'd expect it to jump me to the main content. What do you think? |
|
@srambach it seems like VO is flaky on the skip to content in this demo for me for some reason. Sometimes it doesn't go anywhere, sometimes it accurately jumps to main, and sometimes it jumps to the jump links container and I can't figure out why. If you/anyone have any ideas about why that is I am all ears. |
Hmm - I wasn't even trying VO, I was just using the tab key. |
|
Ah, using tab it seems like the page jumps back to the top but focus does seem to stay on the masthead. This seems to be the case in the other jump links demo and some other demos (the primary detail demos as an example) as well from some quick testing. I wonder if this might be a bug with the dashboard wrapper. |
There was a problem hiding this comment.
Looks good! In terms of the skip link issue mentioned, the behavior I'm seeing I believe matches @wise-king-sullyman where focus looks to remain in the masthead/on the skip link button after clicking it.
Otherwise the skip link does work for me, albeit it's difficult to tell since the main content doesn't have a very large scroll (if I scroll in the content area with the "First section" etc headings, the main element scrolls a little and then clicking the skip link jumps the page back up the tiny bit).
If you want to create an issue, that can be something we look at in the future. |
| <DashboardWrapper breadcrumb={null} mainContainerId="scrollable-element"> | ||
| <Drawer isExpanded={isExpanded}> | ||
| <DrawerContent panelContent={panelContent} id="jump-links-drawer-drawer-scrollable-container"> | ||
| <DrawerContentBody> |
There was a problem hiding this comment.
The HTML demo also has this issue:
Would HTML need to be updated to fix that, or since this React demo aligns with HTML would it be fine leaving it as-is?
thatblindgeye
requested review from
nicolethoen
and removed request for
mattnolting
|
Note that if you use the Skip to Content link, the jumplinks stop working. If you think this is because of the bug with Skip to Content noted above, then I guess that issue should be updated; otherwise it would be a new one. |
What: Closes #6582
Additional issues:
One item on this that I'm not sure about: I didn't see how the drawer was to be toggled in the core demo. I took a similar approach to the switch used to demonstrate vertical vs horizontal capabilities in the other JumpLinks demo, but am more than happy to change it.
Convenience link to the jump links demos page: https://patternfly-react-pr-7520.surge.sh/components/jump-links/react-demos
And the new demo itself: https://patternfly-react-pr-7520.surge.sh/components/jump-links/react-demos/with-drawer/