Descriptionlist in drawer demo #10229
Conversation
|
Preview: https://patternfly-react-pr-10229.surge.sh A11y report: https://patternfly-react-pr-10229-a11y.surge.sh |
tlabaj
requested review from
a team,
lboehling,
kaylachumley,
mmenestr and
mattnolting
and removed request for
a team,
lboehling,
kaylachumley,
wise-king-sullyman,
kmcfaul,
mmenestr and
mattnolting
packages/react-core/src/demos/DescriptionList/DescriptionList.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/DescriptionList.md
Outdated
Show resolved
Hide resolved
| const onCloseClick = () => { | ||
| setIsExpanded(false); | ||
| }; | ||
|
|
||
| const onOpenDrawer = () => { | ||
| setIsExpanded(true); | ||
| }; |
There was a problem hiding this comment.
In addition to opening/closing the drawer, we should set focus on the appropriate element.
- For the Drawer onExpand, the close button of the Drawer should receive focus, similar to our basic drawer example
- When the Drawer gets closed, focus should be set back on the element that previously had focus before the Drawer was expanded. In this case it's easier since there's only the one button controlling expansion.
There was a problem hiding this comment.
Can you please help me on first point, is there any props in drawer component for setting focus on close button. In basic drawer example, I can't see the focus on close button.
Second point I've covered.
There was a problem hiding this comment.
Ah yeah so in that example I linked we're focusing the content of the drawer rather than the close button. We should actually do a similar thing here since the close button comes after the content in the DOM. We can just add a ref similar to that basic drawer example.
We'll also want to move the logic you currently have on the "Open drawer" button, tabIndex={isExpanded ? 0 : -1} to the drawer content title.
set focus back on button after closing drawer
packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx
Outdated
Show resolved
Hide resolved
|
Sorry I missed your previous comment @thatblindgeye - it's true that a selectable/deselectable card would be a more accurate way to show a drawer interaction via a card view, rather than just having link in a card. But I think since this demo is for the purpose of showing a description list in a drawer, the interaction is fine as is! If you have extra time on your hands though, I would change it to a clickable card interaction because people get confused when we show different interactions in different places and don't know which one is "correct". And it could just be a single card, doesn't need to be a full page of clickable cards... Will let you guys make the call though! Everything in the drawer looks good though!! |
packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is looking good! We'd probably want to open an issue to update the core demo to match this if this is what we want it to look like (cc @mcoker ).
Can you try force pushing to this PR? For some reason the PR is building oddly in that it's building a drawer based on the latest update and as though we're still creating a Drawer component inside the demo. Pulling the PR locally everything seems fine, though.
I tried force push but nothing happened. Any idea, why a11y job is failing |
|
The a11y tests are failing on this PR: https://patternfly-react-pr-10229-a11y.surge.sh/ because there is a rendered DrawerPanel in a DrawerPanel which both have the
This is happening because you are passing a Drawer component via the |
|
This PR has been automatically marked as stale because it has not had recent activity. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Closing in favor of #11404 which targets our main v6 branch. |
What: Closes #10227
Descriptionlist in drawer demo
Additional issues: