Workspace buttons discloser switched to a Semantic UI menu #585
Open
Workspace buttons discloser switched to a Semantic UI menu #585
Conversation
hoyla
added
workspaces
ux/ui
quick win
Contributor
Author
|
DRAFT: I refactored this while working on the json export #589 and need to merge those improvements in |
Contributor
Author
|
Hold in draft until some other stuff is out of the way |
…icon - Replace custom button/div dropdown with Semantic UI Dropdown component - Switch disclosure icon from expand-more chevron to hamburger (bars) icon - Add controlled open mode (isOpen/onClose) to ModalAction, ShareWorkspaceModal, and TakeOwnershipOfWorkspaceModal so modals can be triggered from menu items - Menu items use Semantic UI Dropdown.Item with icons matching the tree context menu - Take Ownership is hidden for non-admins; Share/Rename/Delete shown disabled for non-owners
The ModalAction components are now used in controlled mode (isOpen/onClose) so their children (trigger buttons) are never rendered. Remove the unused imports and children content, converting to self-closing elements.
The controlled/uncontrolled open state pattern was duplicated across ShareWorkspaceModal, TakeOwnershipOfWorkspaceModal, and ModalAction. Extract it into a reusable useControlledOpen hook in anticipation of further menu options that will introduce additional modals.
hoyla
force-pushed
the
ljh-workspace-extra-buttons-as-menu
branch
from
a7533c2 to
0ef6a7b
Compare
Place the hook alongside ModalAction and other shared UI utilities rather than in a separate hooks/ directory, keeping related code colocated.
There was a problem hiding this comment.
Pull request overview
Replaces the custom button-based dropdown for workspace actions with a Semantic UI Dropdown menu, and introduces a reusable useControlledOpen hook to support both controlled (parent-driven) and uncontrolled (internal state) modal open/close patterns.
Changes:
- Replaced the custom expand-more button/div disclosure with a Semantic UI
Dropdowncomponent using a hamburger icon trigger - Added
isOpen/onClosecontrolled-mode props toModalAction,ShareWorkspaceModal, andTakeOwnershipOfWorkspaceModal, extracted into a shareduseControlledOpenhook - Updated workspace header CSS to use
flex-endandbox-sizing: border-box
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
useControlledOpen.ts |
New reusable hook for controlled/uncontrolled open state |
WorkspaceSummary.tsx |
Replaced custom dropdown with Semantic UI Dropdown; modals now triggered via menu items with controlled open state |
ModalAction.tsx |
Added controlled open support via useControlledOpen |
ShareWorkspaceModal.tsx |
Added controlled open support via useControlledOpen |
TakeOwnershipOfWorkspaceModal.tsx |
Added controlled open support via useControlledOpen |
_workspace.scss |
Minor layout fixes for workspace header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
In controlled mode, setOpen(true) was writing to internalOpen which is never read (open derives from props.isOpen). Remove the dead state write so the controlled branch only delegates to onClose.
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
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.
I hope to add some more options for workspaces and I think having a stack of buttons displayed vertically will start to look strange if we keep adding more, so I've changed the discloser that shows more buttons ...
... to a regular dropdown menu, which will more suitably handle additions:
Permissions/context-based UX remains the same: don't show an option if you can never do something (like Take Ownership for non-admins); display but disable if you could do something if you owned the workspace (like Share/Rename).
Tested locally and on playground with no noticeable ill effects