-
Notifications
You must be signed in to change notification settings - Fork 48
Add reusable tabs #1462
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.
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
base: main
Are you sure you want to change the base?
Add reusable tabs #1462
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new reusable ManagedTabs component to standardize tab interfaces across the application, replacing previous implementations in workspaces, users, and datasets pages. The new component provides consistent tab layout, overflow handling, and action button integration.
Key changes:
- Created a new
ManagedTabscomponent with overflow management and action button support - Refactored workspace and dataset tab implementations to use the new component
- Consolidated tab styling from multiple files into the new component
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
managed-tabs.module.scss |
New stylesheet defining tab layout, selection indicators, and overflow behavior for the reusable tabs component |
managed-tabs.component.tsx |
New component implementing reusable tab functionality with configurable overflow, action buttons, and custom rendering |
custom-tab-item.module.scss |
Removed duplicated tab styling that's now centralized in managed-tabs |
workspace-users-toolbar.component.tsx |
Refactored to use ManagedTabs instead of custom Tabs implementation |
project-dataset.component.tsx |
Replaced custom tab list with ManagedTabs, integrating overflow and export functionality |
dataset-tab-list.component.tsx |
Removed file as functionality is now provided by ManagedTabs |
workspaces-tabs.component.tsx |
Refactored to use ManagedTabs, extracting WorkspaceItem as separate component |
landing-page.module.scss |
Removed unused styles now handled by ManagedTabs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
web_ui/src/pages/project-details/components/project-dataset/project-dataset.component.tsx
Outdated
Show resolved
Hide resolved
MarkRedeman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general feedback, I think it would be nicer if we could have a component interface that looked more like Spectrum's tabs but with decorated behaviour, something like this,
<Tabs aria-label="History of Ancient Rome">
<ManagedTabList>
<ManagedTabItem key="FoR">Founding of Rome</ManagedTabItem>
<ManagedTabItem key="MaR">Monarchy and Republic</ManagedTabItem>
<ManagedTabItem key="Emp">Empire</ManagedTabItem>
</ManagedTabList>
<TabPanels>
<Item key="FoR">
Arma virumque cano, Troiae qui primus ab oris.
</Item>
<Item key="MaR">
Senatus Populusque Romanus.
</Item>
<Item key="Emp">
Alea jacta est.
</Item>
</TabPanels>
</Tabs>
A good example of this is also the customizing layout from the spectrum docs where they show you can add extra behaviour on the tabs by extending it, not modifying (e.g. open closed principle applies here)
I'd prefer rendering the TabPanels and their contents to be the responsibility of the consumer of this component, as otherwise you might end up with needing to update ManagedTabs every time we have a design that slightly differs from its current behaviour.
| import { useWorkspacesTabs } from './hooks/use-pinned-collapsed-workspace.hook'; | ||
|
|
||
| import classes from '../../../shared/components/custom-tab-item/custom-tab-item.module.scss'; | ||
| const WorkspaceItem = ({ workspace }: { workspace: WorkspaceEntity }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I'd rename this to WorkspaceTabItem, just to emphasize it will render a tab item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| addButton={ | ||
| FEATURE_FLAG_WORKSPACE_ACTIONS | ||
| ? { | ||
| id: 'create-new-workspace-id', | ||
| ariaLabel: 'Create new workspace', | ||
| tooltipText: 'Create a new workspace', | ||
| onPress: createWorkspaceDialogState.open, | ||
| isLoading: createWorkspace.isPending, | ||
| } | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a send as a ReactNode, instead of giving ManagedTabs knowledge about an "add button". Perhaps we could provide this as a suffix prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, still calling it addButton though. Because i guess we always want to add an item, with suffix i think it's unclear of what we're "suffixing"
| renderTabPanel={() => ( | ||
| <HasPermission | ||
| operations={[OPERATION.CAN_SEE_WORKSPACE]} | ||
| specialCondition={!FEATURE_FLAG_WORKSPACE_ACTIONS || undefined} | ||
| Fallback={<NoPermissionPlaceholder />} | ||
| > | ||
| <Workspace /> | ||
| </HasPermission> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we rename this to children, so that you'd do:
<ManagedTabs ...><Workspace /></ManagedTabs/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
c29354e to
8607e4e
Compare
📝 Description
✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: