storage: Add tooltips to Reclaim dialog partition action icons#1204
Merged
KKoukiou merged 1 commit intorhinstaller:mainfrom Apr 14, 2026
Merged
storage: Add tooltips to Reclaim dialog partition action icons#1204KKoukiou merged 1 commit intorhinstaller:mainfrom
KKoukiou merged 1 commit intorhinstaller:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience in the storage reclaim dialog by introducing informative tooltips for the 'Undo', 'Delete', and 'Shrink' partition action icons. It also refines the behavior of the 'Shrink' action's popover, ensuring it remains closed when the action is disabled, thereby preventing unexpected UI interactions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Popover
idremains a static value (idPrefix + "-shrink"), so if multipleShrinkPopoverinstances render on the page this will produce duplicate DOM ids; consider including the device id (as you did for the tooltip) to ensure uniqueness. - The inline
shouldOpen/shouldClosehandlers inShrinkPopoverboth manageisPopoverOpenand call through to the internalshow/hide; consider extracting these into named callbacks or a small helper to keep the control logic consolidated and easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Popover `id` remains a static value (`idPrefix + "-shrink"`), so if multiple `ShrinkPopover` instances render on the page this will produce duplicate DOM ids; consider including the device id (as you did for the tooltip) to ensure uniqueness.
- The inline `shouldOpen`/`shouldClose` handlers in `ShrinkPopover` both manage `isPopoverOpen` and call through to the internal `show`/`hide`; consider extracting these into named callbacks or a small helper to keep the control logic consolidated and easier to reason about.
## Individual Comments
### Comment 1
<location path="src/components/storage/installation-method/ReclaimSpaceModal.jsx" line_range="456" />
<code_context>
};
const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
+ const shrinkButtonRef = useRef(null);
+ const shrinkButtonTooltipId = idPrefix + "-shrink-tooltip-" + device["device-id"].v;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the ShrinkPopover logic by relying on PatternFly’s built-in popover behavior and a single tooltip-wrapped button instead of managing refs and explicit open/close state.
You can keep the new UX (tooltip + preventing popover when disabled) with much less state and wiring by avoiding `triggerRef` and manual `isPopoverOpen` control.
Instead of controlling the popover’s visibility and syncing it with a separate tooltip, let PatternFly handle popover state and simply:
- Use `isAriaDisabled` to decide whether to wrap the button in a `Popover` at all.
- Wrap the button in a `Tooltip` in both cases for the “Resize partition” text.
That removes `useRef`, `shrinkButtonTooltipId`, `isPopoverOpen`, `shouldOpen`, `shouldClose`, and `onHide` entirely.
Example refactor:
```jsx
const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
const [value, setValue] = useState(device.total.v);
const originalValue = cockpit.format_bytes(device.total.v, { separate: true })[0];
const originalUnit = cockpit.format_bytes(device.total.v, { separate: true })[1];
const [inputValue, setInputValue] = useState(originalValue);
const normalizedValue = inputValue.toString().replace(",", ".");
const buttonWithTooltip = (
<Tooltip content={_("Resize partition")}>
<Button
variant="plain"
isAriaDisabled={isAriaDisabled}
icon={<CompressArrowsAltIcon />}
aria-label={_("shrink")}
/>
</Tooltip>
);
// When disabled, show only the tooltip + button, no popover to open
if (isAriaDisabled) {
return buttonWithTooltip;
}
return (
<Popover
aria-label={_("shrink")}
id={idPrefix + "-shrink"}
hasAutoWidth
bodyContent={() => (
<Flex
alignItems={{ default: "alignItemsFlexStart" }}
spaceItems={{ default: "spaceItemsMd" }}
>
{/* existing Slider / InputGroup / Resize button as before */}
</Flex>
)}
>
{buttonWithTooltip}
</Popover>
);
};
```
This preserves:
- Tooltip “Resize partition” on hover/focus.
- Popover never opens when `isAriaDisabled` is true.
- Popover open/close behavior managed by PatternFly, with no extra state or ref management.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -447,6 +454,9 @@ const DeviceActionShrink = ({ device, hasBeenRemoved, newDeviceSize, onAction }) | |||
| }; | |||
|
|
|||
| const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => { | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the ShrinkPopover logic by relying on PatternFly’s built-in popover behavior and a single tooltip-wrapped button instead of managing refs and explicit open/close state.
You can keep the new UX (tooltip + preventing popover when disabled) with much less state and wiring by avoiding triggerRef and manual isPopoverOpen control.
Instead of controlling the popover’s visibility and syncing it with a separate tooltip, let PatternFly handle popover state and simply:
- Use
isAriaDisabledto decide whether to wrap the button in aPopoverat all. - Wrap the button in a
Tooltipin both cases for the “Resize partition” text.
That removes useRef, shrinkButtonTooltipId, isPopoverOpen, shouldOpen, shouldClose, and onHide entirely.
Example refactor:
const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
const [value, setValue] = useState(device.total.v);
const originalValue = cockpit.format_bytes(device.total.v, { separate: true })[0];
const originalUnit = cockpit.format_bytes(device.total.v, { separate: true })[1];
const [inputValue, setInputValue] = useState(originalValue);
const normalizedValue = inputValue.toString().replace(",", ".");
const buttonWithTooltip = (
<Tooltip content={_("Resize partition")}>
<Button
variant="plain"
isAriaDisabled={isAriaDisabled}
icon={<CompressArrowsAltIcon />}
aria-label={_("shrink")}
/>
</Tooltip>
);
// When disabled, show only the tooltip + button, no popover to open
if (isAriaDisabled) {
return buttonWithTooltip;
}
return (
<Popover
aria-label={_("shrink")}
id={idPrefix + "-shrink"}
hasAutoWidth
bodyContent={() => (
<Flex
alignItems={{ default: "alignItemsFlexStart" }}
spaceItems={{ default: "spaceItemsMd" }}
>
{/* existing Slider / InputGroup / Resize button as before */}
</Flex>
)}
>
{buttonWithTooltip}
</Popover>
);
};This preserves:
- Tooltip “Resize partition” on hover/focus.
- Popover never opens when
isAriaDisabledis true. - Popover open/close behavior managed by PatternFly, with no extra state or ref management.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds tooltips to the action icons in the Reclaim Space dialog, which is a good user experience improvement. The implementation correctly uses PatternFly's Tooltip and controlled Popover components to achieve the desired behavior, especially in handling the case where a popover trigger should be disabled. I have one suggestion to simplify the code for controlling the popover's visibility, making it more concise and idiomatic for a controlled React component.
- Attach resize tooltip via triggerRef instead of wrapping the button, so the Popover still opens on click (wrapping with Tooltip breaks the Popover trigger). - Use controlled Popover (isVisible + shouldOpen/shouldClose) so the shrink popover does not open when the button is aria-disabled. PatternFly Popover still fires onTriggerClick for aria-disabled triggers, so we only call show() from shouldOpen when !isAriaDisabled. Resolves: rhbz#2403027
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ShrinkPopover, theshouldOpen/shouldClosehandlers mix local state updates with calling theshow/hidecallbacks inline; consider extracting these into small named functions (or usingonShow/onHideif available) to make the open/close behavior easier to follow and less error-prone. - The shrink button sets
aria-describedbyexplicitly while also using aTooltipwithtriggerRef; double-check whether the Tooltip already wires uparia-describedbyfor the trigger, and if so, drop the manual attribute to avoid redundant or conflicting ARIA metadata. - If the
deviceprop forShrinkPopovercan change while the popover is open (e.g. row removal or data refresh), consider resettingisPopoverOpenin auseEffecttied to the device identifier so that the UI does not show a resize popover for a stale or removed partition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ShrinkPopover`, the `shouldOpen`/`shouldClose` handlers mix local state updates with calling the `show`/`hide` callbacks inline; consider extracting these into small named functions (or using `onShow`/`onHide` if available) to make the open/close behavior easier to follow and less error-prone.
- The shrink button sets `aria-describedby` explicitly while also using a `Tooltip` with `triggerRef`; double-check whether the Tooltip already wires up `aria-describedby` for the trigger, and if so, drop the manual attribute to avoid redundant or conflicting ARIA metadata.
- If the `device` prop for `ShrinkPopover` can change while the popover is open (e.g. row removal or data refresh), consider resetting `isPopoverOpen` in a `useEffect` tied to the device identifier so that the UI does not show a resize popover for a stale or removed partition.
## Individual Comments
### Comment 1
<location path="test/helpers/storage.py" line_range="540-544" />
<code_context>
- disabled = ":disabled" if disabled else ":not(:disabled)"
- else:
- disabled = "[aria-disabled='true']" if disabled else ":not([aria-disabled='true'])"
+ disabled_sel = "[aria-disabled='true']" if disabled else ":not([aria-disabled='true'])"
selector = (
"#reclaim-space-modal-table "
f"tr:contains('{device}') "
- f"button[aria-label='{action}']{disabled}"
+ f"button[aria-label='{action}']{disabled_sel}"
)
</code_context>
<issue_to_address>
**issue (testing):** Add tests to cover shrink popover behavior when the button is aria-disabled and when enabled
To validate the bugfix, please add tests that cover both states:
1. When `isAriaDisabled=true`, clicking the shrink button should not open the popover (no popover content or resize controls rendered).
2. When the shrink action is enabled, clicking the button should open the popover and render the resize controls.
These can be implemented as UI tests that use this helper to select the button, trigger a click, and assert the presence/absence of the popover content in the DOM.
</issue_to_address>
### Comment 2
<location path="src/components/storage/installation-method/ReclaimSpaceModal.jsx" line_range="456" />
<code_context>
};
const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
+ const shrinkButtonRef = useRef(null);
+ const shrinkButtonTooltipId = idPrefix + "-shrink-tooltip-" + device["device-id"].v;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `ShrinkPopover` by letting `Popover` handle visibility and using the same Tooltip-wrapped Button pattern as other actions to avoid extra state and refs.
You can keep all the new UX (tooltip + disabled handling) while dropping most of the extra state/refs in `ShrinkPopover`.
Specifically:
- Let `Popover` manage its own visibility (remove `isPopoverOpen`, `shouldOpen`, `shouldClose`, `onHide`).
- Use the same `<Tooltip><Button/></Tooltip>` pattern you used for Undo/Delete instead of `triggerRef` + `aria-describedby`.
- You no longer need `useRef`, `shrinkButtonRef`, or `shrinkButtonTooltipId`.
A simplified `ShrinkPopover` preserving behavior would look like:
```jsx
const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
const [value, setValue] = useState(device.total.v);
const originalValue = cockpit.format_bytes(device.total.v, { separate: true })[0];
const originalUnit = cockpit.format_bytes(device.total.v, { separate: true })[1];
const [inputValue, setInputValue] = useState(originalValue);
const normalizedValue = inputValue.toString().replace(",", ".");
const shrinkButton = (
<Tooltip content={_("Resize partition")}>
<Button
variant="plain"
isAriaDisabled={isAriaDisabled}
icon={<CompressArrowsAltIcon />}
aria-label={_("shrink")}
/>
</Tooltip>
);
return (
<Popover
aria-label={_("shrink")}
id={idPrefix + "-shrink"}
hasAutoWidth
bodyContent={() => (
<Flex
alignItems={{ default: "alignItemsFlexStart" }}
spaceItems={{ default: "spaceItemsMd" }}
>
<Slider
areCustomStepsContinuous
className={idPrefix + "-shrink-slider"}
id={idPrefix + "-shrink-slider"}
inputLabel={originalUnit}
value={(value * 100) / device.total.v}
showBoundaries={false}
onChange={(_, sliderValue) => {
const newValue = Math.round((sliderValue / 100) * device.total.v);
setValue(newValue);
setInputValue(
cockpit.format_bytes(newValue, originalUnit, { separate: true })[0]
);
}}
customSteps={[
{ label: "0", value: 0 },
{ label: cockpit.format_bytes(device.total.v), value: 100 },
]}
/>
<InputGroup>
<InputGroupItem>
<TextInput
value={inputValue}
onChange={(_event, val) => setInputValue(val)}
onBlur={() => {
const newValue = Math.min(
device.total.v,
Math.max(0, normalizedValue * unitMultiplier[originalUnit]),
);
if (Number.isNaN(newValue)) {
setInputValue(
cockpit.format_bytes(
value,
originalUnit,
{ separate: true },
)[0],
);
return;
}
setValue(newValue);
setInputValue(
cockpit.format_bytes(newValue, originalUnit, {
separate: true,
})[0],
);
}}
id={idPrefix + "-shrink-input"}
/>
</InputGroupItem>
<InputGroupText>{originalUnit}</InputGroupText>
</InputGroup>
<Button
id={idPrefix + "-shrink-button"}
variant="primary"
isAriaDisabled={value === 0 || value === device.total.v}
onClick={() => onShrink(value)}
>
{_("Resize")}
</Button>
</Flex>
)}
>
{shrinkButton}
</Popover>
);
};
```
This keeps:
- Tooltip text and behavior.
- Disabled state when `isAriaDisabled` is true.
- All existing resize logic.
While removing:
- `useRef` / `triggerRef`.
- `shrinkButtonTooltipId` and `aria-describedby`.
- The extra `isPopoverOpen` state and custom `shouldOpen` / `shouldClose` wiring.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+540
to
+544
| disabled_sel = "[aria-disabled='true']" if disabled else ":not([aria-disabled='true'])" | ||
| selector = ( | ||
| "#reclaim-space-modal-table " | ||
| f"tr:contains('{device}') " | ||
| f"button[aria-label='{action}']{disabled}" | ||
| f"button[aria-label='{action}']{disabled_sel}" |
There was a problem hiding this comment.
issue (testing): Add tests to cover shrink popover behavior when the button is aria-disabled and when enabled
To validate the bugfix, please add tests that cover both states:
- When
isAriaDisabled=true, clicking the shrink button should not open the popover (no popover content or resize controls rendered). - When the shrink action is enabled, clicking the button should open the popover and render the resize controls.
These can be implemented as UI tests that use this helper to select the button, trigger a click, and assert the presence/absence of the popover content in the DOM.
| @@ -447,6 +454,9 @@ const DeviceActionShrink = ({ device, hasBeenRemoved, newDeviceSize, onAction }) | |||
| }; | |||
|
|
|||
| const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => { | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying ShrinkPopover by letting Popover handle visibility and using the same Tooltip-wrapped Button pattern as other actions to avoid extra state and refs.
You can keep all the new UX (tooltip + disabled handling) while dropping most of the extra state/refs in ShrinkPopover.
Specifically:
- Let
Popovermanage its own visibility (removeisPopoverOpen,shouldOpen,shouldClose,onHide). - Use the same
<Tooltip><Button/></Tooltip>pattern you used for Undo/Delete instead oftriggerRef+aria-describedby. - You no longer need
useRef,shrinkButtonRef, orshrinkButtonTooltipId.
A simplified ShrinkPopover preserving behavior would look like:
const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
const [value, setValue] = useState(device.total.v);
const originalValue = cockpit.format_bytes(device.total.v, { separate: true })[0];
const originalUnit = cockpit.format_bytes(device.total.v, { separate: true })[1];
const [inputValue, setInputValue] = useState(originalValue);
const normalizedValue = inputValue.toString().replace(",", ".");
const shrinkButton = (
<Tooltip content={_("Resize partition")}>
<Button
variant="plain"
isAriaDisabled={isAriaDisabled}
icon={<CompressArrowsAltIcon />}
aria-label={_("shrink")}
/>
</Tooltip>
);
return (
<Popover
aria-label={_("shrink")}
id={idPrefix + "-shrink"}
hasAutoWidth
bodyContent={() => (
<Flex
alignItems={{ default: "alignItemsFlexStart" }}
spaceItems={{ default: "spaceItemsMd" }}
>
<Slider
areCustomStepsContinuous
className={idPrefix + "-shrink-slider"}
id={idPrefix + "-shrink-slider"}
inputLabel={originalUnit}
value={(value * 100) / device.total.v}
showBoundaries={false}
onChange={(_, sliderValue) => {
const newValue = Math.round((sliderValue / 100) * device.total.v);
setValue(newValue);
setInputValue(
cockpit.format_bytes(newValue, originalUnit, { separate: true })[0]
);
}}
customSteps={[
{ label: "0", value: 0 },
{ label: cockpit.format_bytes(device.total.v), value: 100 },
]}
/>
<InputGroup>
<InputGroupItem>
<TextInput
value={inputValue}
onChange={(_event, val) => setInputValue(val)}
onBlur={() => {
const newValue = Math.min(
device.total.v,
Math.max(0, normalizedValue * unitMultiplier[originalUnit]),
);
if (Number.isNaN(newValue)) {
setInputValue(
cockpit.format_bytes(
value,
originalUnit,
{ separate: true },
)[0],
);
return;
}
setValue(newValue);
setInputValue(
cockpit.format_bytes(newValue, originalUnit, {
separate: true,
})[0],
);
}}
id={idPrefix + "-shrink-input"}
/>
</InputGroupItem>
<InputGroupText>{originalUnit}</InputGroupText>
</InputGroup>
<Button
id={idPrefix + "-shrink-button"}
variant="primary"
isAriaDisabled={value === 0 || value === device.total.v}
onClick={() => onShrink(value)}
>
{_("Resize")}
</Button>
</Flex>
)}
>
{shrinkButton}
</Popover>
);
};This keeps:
- Tooltip text and behavior.
- Disabled state when
isAriaDisabledis true. - All existing resize logic.
While removing:
useRef/triggerRef.shrinkButtonTooltipIdandaria-describedby.- The extra
isPopoverOpenstate and customshouldOpen/shouldClosewiring.
adamkankovsky
approved these changes
Apr 7, 2026
Contributor
adamkankovsky
left a comment
adamkankovsky
left a comment
There was a problem hiding this comment.
Looks great to me. Thanks
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
2 participants
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.
Resolves: rhbz#2403027