[LG-5481] feat(drawer): toggle drawer based on toolbar item visibility #3085
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
… flicker handling
…hide-drawer-toolbar
…tate for embedded drawers
…hide-drawer-toolbar
…toolbar and panel content
…lity and item management
…or drawer visibility based on active item
…e and simplify story implementations
🦋 Changeset detectedLatest commit: 6cbabcc 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 implements functionality to automatically close the drawer when the currently active toolbar item's visibility is set to false. The change adds a new useEffect hook that monitors toolbar item visibility and triggers drawer closure when needed.
Key changes:
- Added visibility monitoring logic to automatically close drawer when active item becomes hidden
- Refactored test utilities to support more flexible toolbar data manipulation
- Added comprehensive test coverage and Storybook stories for the new behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
DrawerToolbarLayoutContent.tsx |
Added useEffect hook to monitor active toolbar item visibility and close drawer when item becomes hidden |
DrawerToolbarLayout.testutils.tsx |
Refactored utilities with new useToolbarData hook and improved parameter naming for better flexibility |
DrawerToolbarLayout.stories.tsx |
Updated stories to use new test utilities and added button for testing toolbar item visibility |
DrawerToolbarLayout.spec.tsx |
Added unit test to verify drawer closes when active item is hidden |
DrawerToolbarLayout.interactions.stories.tsx |
Added comprehensive interaction stories testing the new visibility behavior |
| @@ -59,6 +59,17 @@ export const DrawerToolbarLayoutContent = forwardRef< | |||
|
|
|||
| const shouldRenderToolbar = visibleToolbarItems.length > 0; | |||
|
|
|||
| // If there is an active toolbar item and it's visibility is set to false, close the drawer | |||
There was a problem hiding this comment.
Grammar error: 'it's visibility' should be 'its visibility' (possessive, not contraction).
Suggested change
| // If there is an active toolbar item and it's visibility is set to false, close the drawer | |
| // If there is an active toolbar item and its visibility is set to false, close the drawer |
Copilot uses AI. Check for mistakes.
Comment on lines
63
to
71
| useEffect(() => { | ||
| if (id && toolbarData && isDrawerOpen && shouldRenderToolbar) { | ||
| const activeToolbarItem = toolbarData.find(item => item.id === id); | ||
|
|
||
| if (activeToolbarItem && activeToolbarItem.visible === false) { | ||
| closeDrawer(); | ||
| } | ||
| } | ||
| }, [id, toolbarData, closeDrawer, isDrawerOpen, shouldRenderToolbar]); |
There was a problem hiding this comment.
The useEffect runs on every change to shouldRenderToolbar, but this dependency is unnecessary since the effect only needs to run when the specific active toolbar item's visibility changes. Consider removing shouldRenderToolbar from the dependency array or restructuring the logic to avoid unnecessary re-runs.
Suggested change
| useEffect(() => { | |
| if (id && toolbarData && isDrawerOpen && shouldRenderToolbar) { | |
| const activeToolbarItem = toolbarData.find(item => item.id === id); | |
| if (activeToolbarItem && activeToolbarItem.visible === false) { | |
| closeDrawer(); | |
| } | |
| } | |
| }, [id, toolbarData, closeDrawer, isDrawerOpen, shouldRenderToolbar]); | |
| }, [id, toolbarData, closeDrawer, isDrawerOpen]); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think we do need to have shouldRenderToolbar as a dependency. This should only run if the toolbar is visible.
|
Size Change: +99 B (0%) Total Size: 2.01 MB
ℹ️ View Unchanged
|
stephl3
approved these changes
Aug 28, 2025
Comment on lines
62
to
71
| // If the drawer is open, there is an active toolbar item, it's visibility is set to false, and the toolbar is visible, close the drawer. | ||
| useEffect(() => { | ||
| if (id && toolbarData && isDrawerOpen && shouldRenderToolbar) { | ||
| const activeToolbarItem = toolbarData.find(item => item.id === id); | ||
| const isActiveToolbarItemVisible = activeToolbarItem?.visible ?? true; | ||
|
|
||
| if (activeToolbarItem && !isActiveToolbarItemVisible) { | ||
| closeDrawer(); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: can improve readability by inverting the if blocks and early returning and/or adding these comments inline
…ygreen-ui into LG-5481/drawer-toolbar-item-toggle
…e toolbar item visibility
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
This PR implements functionality to automatically close the drawer when the currently active toolbar item's visibility is set to false. The change adds a new useEffect hook that monitors toolbar item visibility and triggers drawer closure when needed.
Key changes:
🎟 Jira ticket: LG-5481
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changes🧪 How to test changes