-
Notifications
You must be signed in to change notification settings - Fork 53
feat: [WD-32447] Rich tooltips for pools #1698
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?
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 rich tooltips for storage pools throughout the LXD-UI application. The new StoragePoolRichChip component replaces simple ResourceLink components with an interactive tooltip that displays detailed pool information (description, status, driver, and used-by count) when users hover over pool references.
Key Changes:
- Created new
StoragePoolRichChipandStoragePoolRichTooltipcomponents with associated styling - Replaced
ResourceLinkcomponents withStoragePoolRichChipacross 12 files for consistent pool display - Added responsive behavior to hide tooltips on smaller screens
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/pages/storage/StoragePoolRichChip.tsx |
New component that wraps pool links with rich tooltip functionality |
src/pages/storage/StoragePoolRichTooltip.tsx |
New component that renders the tooltip content with pool details |
src/sass/_storage_pool_rich_tooltip.scss |
Styling for the rich tooltip table layout |
src/sass/styles.scss |
Imports the new stylesheet |
src/util/instanceMigration.tsx |
Replaced ResourceLink with StoragePoolRichChip in migration success message |
src/pages/storage/StorageVolumes.tsx |
Updated pool column to use StoragePoolRichChip |
src/pages/storage/StorageVolumeOverview.tsx |
Updated pool display in overview table |
src/pages/storage/StoragePoolHeader.tsx |
Updated pool reference in rename success message |
src/pages/storage/StorageBuckets.tsx |
Updated pool column to use StoragePoolRichChip |
src/pages/storage/MigrateVolumeBtn.tsx |
Updated pool references in migration notifications |
src/pages/storage/EditStoragePool.tsx |
Updated pool reference in edit success message |
src/pages/storage/CreateStoragePool.tsx |
Updated pool reference in creation success message |
src/pages/profiles/ProfileStorageList.tsx |
Updated pool display in profile storage device list |
src/pages/instances/InstanceDetailPanelContent.tsx |
Updated root storage pool display |
src/components/DeviceDetails.tsx |
Updated pool references in device details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9547f26 to
d9b15bb
Compare
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
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d9b15bb to
5b355ce
Compare
|
On your screenshot, the instance icon in the Used by section doesn't have the same color as the other icons. |
|
I thought we agreed yesterday not to use |
5b355ce to
e5c29ec
Compare
omarelkashef
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.
Really nice! some observations below
56081b8 to
4e591a3
Compare
|
@edlerd , could you please let me know where you wanted the PoolRichChips with the Memory bar to be? There are quite a lot of layers of abstraction, eg, in order to display the memory bar in the poolchip on the instance page, the cluster member has to go from InstanceOverview to DeviceDetails OR, be called with a hook within Device details. What do you think? |
omarelkashef
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.
Thank you for the improvements! A few more comments below
| @@ -0,0 +1,83 @@ | |||
| import { type FC } from "react"; | |||
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.
If we can not find other rows for the tooltip, maybe having an exception and overriding the min-height is our best choice because the spacing does not look right. What do you think?
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.
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.
We discussed earlier this week having an exception for the pool tooltip due to a lack of things to show in it.
Let's forward a |
edlerd
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.
I think there was a misunderstanding between cluster member memory and storage pool size. See below.
No, as the URL needs to access the device in the ..map and filter. |
4e591a3 to
afecfee
Compare
Signed-off-by: Nkeiruka <[email protected]>
afecfee to
a1fd971
Compare
| return member && location ? ( | ||
| <ClusterMemberMemoryUsage member={member} /> | ||
| ) : ( | ||
| <StoragePoolSize pool={pool} /> |
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.
|
|
||
| const StoragePoolRichTooltipSize: FC<Props> = ({ pool, member, location }) => { | ||
| return member && location ? ( | ||
| <ClusterMemberMemoryUsage member={member} /> |
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.
We want the storage pool size for a cluster member, but only if the pool is local and the environment is clustered and if the pool is local. This is the wrong component, pulling the cluster member memory. It should be the storage pool usage for the given cluster member instead.
If no cluster member is given here, we check if there is only one cluster member. If there is only one, we show the pool usage for the pool on that member
If the pool is not having a isClusterLocalDriver then StoragePoolSize is already doing the right thing.
If no cluster member is given here and there is more than one cluster member and isClusterLocalDriver for that pool driver, then we should show a fallback.
| @@ -0,0 +1,83 @@ | |||
| import { type FC } from "react"; | |||
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.
We discussed earlier this week having an exception for the pool tooltip due to a lack of things to show in it.



Done
Notes:
QA
Screenshots