Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(Page): add prop to set width of drawer #10279

Merged
merged 3 commits into from
Apr 26, 2024
Merged

feat(Page): add prop to set width of drawer #10279

merged 3 commits into from
Apr 26, 2024

Conversation

jessiehuff
Copy link
Contributor

What: Closes #10133

@jessiehuff jessiehuff changed the title Add drawerWidth prop feat(Page): add prop to set width of drawer Apr 15, 2024
@patternfly-build
Copy link
Contributor

patternfly-build commented Apr 15, 2024 •
edited
Loading

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

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

tlabaj
tlabaj reviewed Apr 16, 2024
View reviewed changes
packages/react-core/src/components/Page/Page.tsx Outdated
@@ -302,6 +305,7 @@ class Page extends React.Component<PageProps, PageState> {
className={css(styles.pageMain)}
tabIndex={mainTabIndex}
aria-label={mainAriaLabel}
style={{ width: `${drawerWidth}` }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcoker do we want to set the --pf-v5-c-drawer__panel--md--FlexBasis css var here instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Screenshot 2024-04-17 at 3 23 47 PM

@tlabaj tlabaj requested a review from mcoker April 16, 2024 14:46
mcoker
mcoker reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!! Left a few comments for review 👍

packages/react-core/src/components/Page/Page.tsx Outdated Show resolved Hide resolved
packages/react-core/src/components/Page/Page.tsx Outdated
@@ -302,6 +305,7 @@ class Page extends React.Component<PageProps, PageState> {
className={css(styles.pageMain)}
tabIndex={mainTabIndex}
aria-label={mainAriaLabel}
style={{ width: `${drawerWidth}` }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Screenshot 2024-04-17 at 3 23 47 PM

packages/react-core/src/components/Page/Page.tsx Outdated Show resolved Hide resolved
@jessiehuff jessiehuff requested a review from mcoker April 22, 2024 15:27
mcoker
mcoker reviewed Apr 23, 2024
View reviewed changes
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

packages/react-core/src/components/Drawer/DrawerPanelContent.tsx Outdated
@@ -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. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - should this comment match the others?

Suggested change
/** The minimum size of a drawer, in either pixels or percentage. */
/** The minimum size of a drawer. */

packages/react-core/src/components/Drawer/DrawerPanelContent.tsx Outdated
@@ -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. */
minSize?: string;
/** The starting size of a resizable drawer, in either pixels or percentage. */
/** The starting size of a resizable drawer. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

packages/react-core/src/components/Page/Page.tsx Outdated
@@ -27,6 +27,12 @@ export interface PageProps extends React.HTMLProps<HTMLDivElement> {
notificationDrawer?: React.ReactNode;
/** Flag indicating Notification drawer in expanded */
isNotificationDrawerExpanded?: boolean;
/** Sets default drawer width size */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sets vertical drawers, too, so we can probably just leave off "width"

Suggested change
/** Sets default drawer width size */
/** Sets default drawer size */

packages/react-core/src/components/Page/Page.tsx Outdated
@@ -27,6 +27,12 @@ export interface PageProps extends React.HTMLProps<HTMLDivElement> {
notificationDrawer?: React.ReactNode;
/** Flag indicating Notification drawer in expanded */
isNotificationDrawerExpanded?: boolean;
/** Sets default drawer width size */
drawerDefaultSize?: string;
/** The starting size of a resizable drawer */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we match the wording of the default prop

Suggested change
/** The starting size of a resizable drawer */
/** Sets the minimum drawer size */

packages/react-core/src/components/Page/Page.tsx Outdated
drawerDefaultSize?: string;
/** The starting size of a resizable drawer */
drawerMinSize?: string;
/** The maximum size of a drawer */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** The maximum size of a drawer */
/** Sets the max drawer size */

tlabaj
tlabaj approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mcoker
mcoker approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎃

@tlabaj tlabaj merged commit 24df4bd into patternfly:main Apr 26, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmcfaul kmcfaul kmcfaul approved these changes

@tlabaj tlabaj tlabaj approved these changes

@mcoker mcoker mcoker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Page: Add a prop to set the width if the drawer
5 participants
@jessiehuff @patternfly-build @mcoker @tlabaj @kmcfaul