[LG-5460] feat(drawer): add toolbar item visibility control #3067
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hide-drawer-toolbar
…ggle functionality
…or mobile responsiveness
…etter drawer integration
…hide-drawer-toolbar
🦋 Changeset detectedLatest commit: 8017dd5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the drawer component by adding dynamic toolbar visibility control and layout consistency improvements. The key enhancement is the addition of a visible property to toolbar items that allows for conditional rendering of toolbar elements.
- New visible property: Toolbar items can now be conditionally shown/hidden using a
visibleboolean property (defaults to true) - Dynamic toolbar rendering: When all toolbar items have
visible: false, the entire toolbar element is removed from the DOM - Layout consolidation: Replaced
DrawerWithToolbarWrapperwith a unifiedLayoutGridcomponent for consistent layout handling
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/drawer/src/constants.ts | Updated mobile breakpoint to use design token from LeafyGreen UI |
| packages/drawer/src/LayoutComponent/OverlayDrawerLayout/OverlayDrawerLayout.tsx | Added drawer prop support and integrated LayoutGrid for consistent layout |
| packages/drawer/src/LayoutComponent/LayoutGrid/LayoutGrid.tsx | New unified layout component replacing DrawerWithToolbarWrapper |
| packages/drawer/src/DrawerToolbarLayout/DrawerToolbarLayout/DrawerToolbarLayoutContent.tsx | Added toolbar visibility logic and integrated new layout structure |
| packages/drawer/src/DrawerToolbarLayout/DrawerToolbarLayout/DrawerToolbarLayout.types.ts | Added visible property to toolbar item interface |
| packages/drawer/src/DrawerLayout/DrawerLayoutContext/DrawerLayoutContext.tsx | Added setHasToolbar function to context for dynamic toolbar state management |
| packages/drawer/src/Drawer/Drawer.tsx | Added width persistence logic for embedded drawers when toolbar is toggled |
|
|
||
| useEffect(() => { | ||
| // This updates the drawer open state when the toolbar is inter |
There was a problem hiding this comment.
The comment appears to be cut off. 'inter' should likely be 'interacted with' or similar complete phrase.
Suggested change
| // This updates the drawer open state when the toolbar is inter | |
| // This updates the drawer open state when the toolbar is interacted with |
Copilot uses AI. Check for mistakes.
shaneeza
changed the title
shaneeza
commented
Aug 21, 2025
There was a problem hiding this comment.
These styles were initially from DrawerWithToolbarWrapper. I used git mv to rename the files, so I'm unsure why it appears as a new file.
shaneeza
commented
Aug 21, 2025
packages/drawer/src/LayoutComponent/OverlayDrawerLayout/OverlayDrawerLayout.tsx
Outdated
Show resolved
Hide resolved
|
Size Change: +75 B (0%) Total Size: 2.01 MB
ℹ️ View Unchanged
|
… flicker handling
stephl3
reviewed
Aug 25, 2025
Comment on lines
3
to
6
| export type LayoutGridProps = Omit<HTMLElementProps<'div'>, 'children'> & { | ||
| drawer: React.ReactNode; | ||
| children: React.ReactNode; | ||
| }; |
There was a problem hiding this comment.
If this was done to make sure children is defined, you may need to wrap ReactNode in a Required<> since ReactNode includes undefined
| }, | ||
| ]; | ||
|
|
||
| export const DRAWER_TOOLBAR_DATA_NOT_VISIBLE: DrawerToolbarLayoutProps['toolbarData'] = |
There was a problem hiding this comment.
if these are the same other than the visible key, this could be simplified to:
Suggested change
| export const DRAWER_TOOLBAR_DATA_NOT_VISIBLE: DrawerToolbarLayoutProps['toolbarData'] = | |
| export const DRAWER_TOOLBAR_DATA_NOT_VISIBLE: DrawerToolbarLayoutProps['toolbarData'] = | |
| DRAWER_TOOLBAR_DATA.map(dataObj => ({ ...dataObj, visible: false })); |
There was a problem hiding this comment.
I ended up creating a function to return data along with this suggesion
Comment on lines
+56
to
+61
| // Calculate visibleToolbarItems in component body (needed for rendering) | ||
| const visibleToolbarItems = toolbarData?.filter( | ||
| toolbarItem => toolbarItem.visible ?? true, | ||
| ); | ||
|
|
||
| const shouldRenderToolbar = visibleToolbarItems.length > 0; |
There was a problem hiding this comment.
I think this was shared before, but I can't remember. What is the specific reason this needs to be derived from checking that all toolbar items have visible={false} as opposed to having a boolean for the Toolbar instance itself?
There was a problem hiding this comment.
Suggested change
| // Calculate visibleToolbarItems in component body (needed for rendering) | |
| const visibleToolbarItems = toolbarData?.filter( | |
| toolbarItem => toolbarItem.visible ?? true, | |
| ); | |
| const shouldRenderToolbar = visibleToolbarItems.length > 0; | |
| // Calculate visibleToolbarItems in component body (needed for rendering) | |
| const shouldRenderToolbar = toolbarData?.every( | |
| toolbarItem => toolbarItem.visible ?? true, | |
| ); |
There was a problem hiding this comment.
There can be instances where some items should be toggled based on the context. In cases like this, the toolbar should still render, but with only visible items.
There was a problem hiding this comment.
gotcha, I missed that we are also rendering it. good to know!
| </Drawer> | ||
| </DrawerWithToolbarWrapper> | ||
| <LayoutGrid | ||
| drawer={ |
There was a problem hiding this comment.
is there an alternative more explicit name that could be used here to better communicate that this could be a Toolbar+Drawer vs just a Drawer?
There was a problem hiding this comment.
I ended up refactoring this. I renamed it to PanelGrid, and its sole responsibility is rendering the components that should be in the panel. It will only accept children. Along with this, I renamed the drawer prop on LayoutComponent to panelContent since this is the content that will be passed to PanelGrid.
Comment on lines
144
to
146
| [getWithoutToolbarBaseStyles({ hasDrawerProp })]: !hasToolbar, | ||
| [getWithoutToolbarOpenStyles({ hasDrawerProp })]: | ||
| isDrawerOpen && !hasToolbar, |
There was a problem hiding this comment.
nit: I think readability of this (and the preceding couple lines) would improve a lot by further consolidating these styles as getClosedStyles + getOpenStyles
Suggested change
| [getWithoutToolbarBaseStyles({ hasDrawerProp })]: !hasToolbar, | |
| [getWithoutToolbarOpenStyles({ hasDrawerProp })]: | |
| isDrawerOpen && !hasToolbar, | |
| [getClosedStyles({ hasDrawer, hasToolbar })]: !isDrawerOpen, | |
| [getOpenStyles({ hasDrawer, hasToolbar })]: isDrawerOpen, |
also thought about getWithToolbarStyles(isDrawerOpen) and getWithoutToolbarStyles({ hasDrawerProp, isDrawerOpen }) but initially forking on isDrawerOpen seems like it better aligns with patterns in other packages
Comment on lines
44
to
48
| {hasDrawerProp ? ( | ||
| <LayoutGrid drawer={drawer}>{children}</LayoutGrid> | ||
| ) : ( | ||
| children | ||
| )} |
There was a problem hiding this comment.
This is a bit confusing to me. Should drawer not be a required prop for this layout?
There was a problem hiding this comment.
The drawer prop is not required because I wanted to keep this backwards compatible. When drawerLayout was first introduced, drawer was not a prop, so consumers had to pass the drawer as a child. With the addition of the drawer prop, the recommendation is to use the drawer prop, but you can still continue to pass the drawer as a child. The difference is how the drawers are rendered internally.
You can also continue to stack overlay drawers with the stack context if the drawers are passed as children.
Comment on lines
32
to
36
| {hasDrawerProp ? ( | ||
| <LayoutGrid drawer={drawer}>{children}</LayoutGrid> | ||
| ) : ( | ||
| children | ||
| )} |
There was a problem hiding this comment.
similarly for this, should drawer be a required prop?
sandynguyenn
approved these changes
Aug 26, 2025
…tate for embedded drawers
stephl3
approved these changes
Aug 26, 2025
Comment on lines
+56
to
+61
| // Calculate visibleToolbarItems in component body (needed for rendering) | ||
| const visibleToolbarItems = toolbarData?.filter( | ||
| toolbarItem => toolbarItem.visible ?? true, | ||
| ); | ||
|
|
||
| const shouldRenderToolbar = visibleToolbarItems.length > 0; |
There was a problem hiding this comment.
gotcha, I missed that we are also rendering it. good to know!
…hide-drawer-toolbar
…toolbar and panel content
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
🎟 Jira ticket: LG-5460
This PR enhances the drawer component by adding dynamic toolbar visibility control, improving drawer width persistence, and standardizing layout consistency across all drawer variants.
Key Features:
visibleproperty: Toolbar items can now be conditionally shown/hidden using avisibleboolean property (defaults to true)visible: false, the entire toolbar element is removed from the DOMTechnical Improvements:
Additional Changes:
setHasToolbarfunction to the drawer layout context for better state management✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changes🧪 How to test changes
Drawer/Toolbar/OverlayToggle Toolbar VisibilitybuttonDrawer/Toolbar/EmbeddedToggle Toolbar VisibilitybuttonView embedded story with toolbar toggle:
Drawer/Toolbar/EmbeddedToggle Toolbar VisibilitybuttonThe width should remain the same
Change the size of the previewicon in storybook. All of the drawers should open the same on mobile.