feat(Page): add prop to set width of drawer#10279
Conversation
jessiehuff
changed the title
|
Preview: https://patternfly-react-pr-10279.surge.sh A11y report: https://patternfly-react-pr-10279-a11y.surge.sh |
| className={css(styles.pageMain)} | ||
| tabIndex={mainTabIndex} | ||
| aria-label={mainAriaLabel} | ||
| style={{ width: `${drawerWidth}` }} |
There was a problem hiding this comment.
@mcoker do we want to set the --pf-v5-c-drawer__panel--md--FlexBasis css var here instead?
There was a problem hiding this comment.
I'm actually not sure what this is doing... it's being added to the page's main element inside of drawer content. Here's a screenshot of drawerWidth set to 500px. The drawer panel gets the correct width, but you can see the page main element is also 500px - is that intentional?
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Nice!! Left a few comments for review 👍
| className={css(styles.pageMain)} | ||
| tabIndex={mainTabIndex} | ||
| aria-label={mainAriaLabel} | ||
| style={{ width: `${drawerWidth}` }} |
There was a problem hiding this comment.
I'm actually not sure what this is doing... it's being added to the page's main element inside of drawer content. Here's a screenshot of drawerWidth set to 500px. The drawer panel gets the correct width, but you can see the page main element is also 500px - is that intentional?
jessiehuff
force-pushed
the
drawer-width
branch
from
8fbbe26 to
80f0343
Compare
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
woot! LGTM, left some meta comments, the only one I think needs to update before merging is for drawerMinSize since the comment is currently "The starting size of a resizable drawer" and should say the minimum drawer size instead.
| @@ -39,11 +39,11 @@ export interface DrawerPanelContentProps extends Omit<React.HTMLProps<HTMLDivEle | |||
| onResize?: (event: MouseEvent | TouchEvent | React.KeyboardEvent, width: number, id: string) => void; | |||
| /** The minimum size of a drawer, in either pixels or percentage. */ | |||
There was a problem hiding this comment.
nit - should this comment match the others?
| /** The minimum size of a drawer, in either pixels or percentage. */ | |
| /** The minimum size of a drawer. */ |
| /** The minimum size of a drawer, in either pixels or percentage. */ | ||
| minSize?: string; | ||
| /** The starting size of a resizable drawer, in either pixels or percentage. */ | ||
| /** The starting size of a resizable drawer. */ |
There was a problem hiding this comment.
Sorry I didn't notice this before, but this says starting size of a resizable drawer, but from what I can tell, this prop sets the size on any drawer (not just resizable)
| notificationDrawer?: React.ReactNode; | ||
| /** Flag indicating Notification drawer in expanded */ | ||
| isNotificationDrawerExpanded?: boolean; | ||
| /** Sets default drawer width size */ |
There was a problem hiding this comment.
It sets vertical drawers, too, so we can probably just leave off "width"
| /** Sets default drawer width size */ | |
| /** Sets default drawer size */ |
| isNotificationDrawerExpanded?: boolean; | ||
| /** Sets default drawer width size */ | ||
| drawerDefaultSize?: string; | ||
| /** The starting size of a resizable drawer */ |
There was a problem hiding this comment.
If we match the wording of the default prop
| /** The starting size of a resizable drawer */ | |
| /** Sets the minimum drawer size */ |
| drawerDefaultSize?: string; | ||
| /** The starting size of a resizable drawer */ | ||
| drawerMinSize?: string; | ||
| /** The maximum size of a drawer */ |
There was a problem hiding this comment.
| /** The maximum size of a drawer */ | |
| /** Sets the max drawer size */ |
jessiehuff
force-pushed
the
drawer-width
branch
from
80f0343 to
269ac65
Compare
5 participants
What: Closes #10133