Descriptionlist in drawer demo #10229

ajaypratap003 · 2024-04-01T17:35:17Z

What: Closes #10227
Descriptionlist in drawer demo

Additional issues:

patternfly-build · 2024-04-01T17:42:41Z

Preview: https://patternfly-react-pr-10229.surge.sh

A11y report: https://patternfly-react-pr-10229-a11y.surge.sh

mcoker

The core demo is setup so that the drawer is attached to the <Page> component's main content area. AFAIK the only way to do that on the react side is to use the <Page notificationDrawer={}> prop, which you can find references to in the notification drawer demos.

Someone on the react side please verify that though, I may just not know how to do it differently.

That will fix this issue where the drawer is attached to a page section (screenshot from the demo)

And should make it look like this (screenshot from the core demo)

packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx

ajaypratap003 · 2024-04-19T13:53:15Z

The core demo is setup so that the drawer is attached to the <Page> component's main content area. AFAIK the only way to do that on the react side is to use notificationDrawer prop, which you can find references to in the notification drawer demos.

Someone on the react side please verify that though, I may just not know how to do it differently.

That will fix this issue where the drawer is attached to a page section (screenshot from the demo)
And should make it look like this (screenshot from the core demo)

I'll try it.

ajaypratap003 · 2024-04-19T14:02:26Z

The core demo is setup so that the drawer is attached to the <Page> component's main content area. AFAIK the only way to do that on the react side is to use notificationDrawer prop, which you can find references to in the notification drawer demos.
Someone on the react side please verify that though, I may just not know how to do it differently.
That will fix this issue where the drawer is attached to a page section (screenshot from the demo)

And should make it look like this (screenshot from the core demo)

I'll try it.

thatblindgeye

One issue with the core demo and this one is that the drawer can't be opened via keyboard, since the Cards beings used in the main page content area aren't interactive in any way.

If the intent is that we want to show how to have cards that can open their own drawer for their own DescriptionList, we should use actionable cards (and optionally ensure each Drawer has a unique title based on the card that was clicked to convey that intent better?). If it's more simply to just show a description list in a drawer, would a single button in the main page area suffice? cc @mmenestr

packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx

thatblindgeye

Couple more comments below, otherwise we would just need input on my previous comments above from design/core

packages/react-core/src/demos/DescriptionList/DescriptionList.md

thatblindgeye · 2024-04-23T13:30:14Z

packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx

+  const onCloseClick = () => {
+    setIsExpanded(false);
+  };
+
+  const onOpenDrawer = () => {
+    setIsExpanded(true);
+  };


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.

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.

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

mcoker

LGTM! One typo and an outstanding question about whether we should include tabs or not.

packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx

mmenestr · 2024-04-25T14:33:01Z

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

thatblindgeye

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.

ajaypratap003 · 2024-05-08T17:04:26Z

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

nicolethoen · 2024-05-08T17:14:11Z

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 id="pf-drawer-panel-0"

This is happening because you are passing a Drawer component via the DashboardWrapper's notificationDrawer prop. But I think it's expecting you to pass the contents of a drawer to the notificationDrawer prop rather than build one yourself.
I think i'd recommend you don't use the notificationDrawer prop but instead, wrap the Page in your Drawer instead?

mattnolting

Nice! Looks like some extra elements are being added

I see @nicolethoen was already on it 😆 !

github-actions · 2024-07-08T11:38:32Z

This PR has been automatically marked as stale because it has not had recent activity.

ajaypratap003 added 4 commits March 21, 2024 20:14

add description list basic demo

8c911f1

fix DashboardWrapper not found issue

36c6945

Merge remote-tracking branch 'upstream/main' into demo-descriptionlist

774eeb4

add descriptionlist in drawer demo

428d280

open drawer on card click

ecdda3e

ajaypratap003 requested review from wise-king-sullyman and kmcfaul April 1, 2024 18:29

tlabaj added ux review labels Apr 17, 2024

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 April 17, 2024 13:43

tlabaj removed ux review labels Apr 18, 2024

tlabaj removed the request for review from a team April 18, 2024 18:58

mcoker requested changes Apr 18, 2024

View reviewed changes

packages/react-core/src/demos/DescriptionList/examples/DescriptionListDrawer.tsx Outdated Show resolved Hide resolved

thatblindgeye requested changes Apr 19, 2024

View reviewed changes

fix drawer height issue

0e81335

ajaypratap003 requested review from thatblindgeye and mcoker April 22, 2024 11:24

thatblindgeye reviewed Apr 23, 2024

View reviewed changes

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

thatblindgeye requested changes Apr 23, 2024

View reviewed changes

remove unused imports and

efa6dbb

set focus back on button after closing drawer

ajaypratap003 requested a review from thatblindgeye April 23, 2024 21:09

mcoker reviewed Apr 23, 2024

View reviewed changes

fix typo

dab63d4

ajaypratap003 requested a review from mcoker April 24, 2024 14:00

add isInline prop in button

438245b

set focus on drawer title

d150b5a

thatblindgeye requested changes Apr 29, 2024

View reviewed changes

fix drawer porps issue

cd2d87f

ajaypratap003 requested a review from thatblindgeye May 6, 2024 17:26

thatblindgeye reviewed May 7, 2024

View reviewed changes

mattnolting requested changes May 8, 2024

View reviewed changes

github-actions bot added the Stale label Jul 8, 2024

nicolethoen removed the Stale label Jul 8, 2024

kmcfaul removed ux review labels Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Descriptionlist in drawer demo #10229

Descriptionlist in drawer demo #10229

ajaypratap003 commented Apr 1, 2024

patternfly-build commented Apr 1, 2024 •

edited

Loading

mcoker left a comment •

edited

Loading

ajaypratap003 commented Apr 19, 2024 •

edited

Loading

ajaypratap003 commented Apr 19, 2024 •

edited

Loading

thatblindgeye left a comment

thatblindgeye left a comment

thatblindgeye Apr 23, 2024

ajaypratap003 Apr 23, 2024

thatblindgeye Apr 25, 2024

mcoker left a comment

mmenestr commented Apr 25, 2024 •

edited

Loading

thatblindgeye left a comment

ajaypratap003 commented May 8, 2024

nicolethoen commented May 8, 2024 •

edited

Loading

mattnolting left a comment •

edited

Loading

github-actions bot commented Jul 8, 2024

Descriptionlist in drawer demo #10229

Are you sure you want to change the base?

Descriptionlist in drawer demo #10229

Conversation

ajaypratap003 commented Apr 1, 2024

patternfly-build commented Apr 1, 2024 • edited Loading

mcoker left a comment • edited Loading

Choose a reason for hiding this comment

ajaypratap003 commented Apr 19, 2024 • edited Loading

ajaypratap003 commented Apr 19, 2024 • edited Loading

thatblindgeye left a comment

Choose a reason for hiding this comment

thatblindgeye left a comment

Choose a reason for hiding this comment

thatblindgeye Apr 23, 2024

Choose a reason for hiding this comment

ajaypratap003 Apr 23, 2024

Choose a reason for hiding this comment

thatblindgeye Apr 25, 2024

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

mmenestr commented Apr 25, 2024 • edited Loading

thatblindgeye left a comment

Choose a reason for hiding this comment

ajaypratap003 commented May 8, 2024

nicolethoen commented May 8, 2024 • edited Loading

mattnolting left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 8, 2024

patternfly-build commented Apr 1, 2024 •

edited

Loading

mcoker left a comment •

edited

Loading

ajaypratap003 commented Apr 19, 2024 •

edited

Loading

ajaypratap003 commented Apr 19, 2024 •

edited

Loading

mmenestr commented Apr 25, 2024 •

edited

Loading

nicolethoen commented May 8, 2024 •

edited

Loading

mattnolting left a comment •

edited

Loading