Skip to content

storage: Add tooltips to Reclaim dialog partition action icons #1204

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.

Sign up for GitHub

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

Merged
KKoukiou merged 1 commit into from
Apr 14, 2026
Merged

storage: Add tooltips to Reclaim dialog partition action icons #1204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 97 additions & 67 deletions src/components/storage/installation-method/ReclaimSpaceModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import cockpit from "cockpit";

import { fmt_to_fragments as fmtToFragments } from "utils";

import React, { useContext, useEffect, useState } from "react";
import React, { useContext, useEffect, useRef, useState } from "react";
import { ActionList } from "@patternfly/react-core/dist/esm/components/ActionList/index.js";
import { Alert } from "@patternfly/react-core/dist/esm/components/Alert/index.js";
import { Button } from "@patternfly/react-core/dist/esm/components/Button/index.js";
Expand All @@ -18,6 +18,7 @@ import { Panel } from "@patternfly/react-core/dist/esm/components/Panel/index.js
import { Popover } from "@patternfly/react-core/dist/esm/components/Popover/index.js";
import { Slider } from "@patternfly/react-core/dist/esm/components/Slider/index.js";
import { TextInput } from "@patternfly/react-core/dist/esm/components/TextInput/index.js";
import { Tooltip } from "@patternfly/react-core/dist/esm/components/Tooltip/index.js";
import { Flex, FlexItem } from "@patternfly/react-core/dist/esm/layouts/Flex/index.js";
import { Stack } from "@patternfly/react-core/dist/esm/layouts/Stack/index.js";
import { CompressArrowsAltIcon } from "@patternfly/react-icons/dist/esm/icons/compress-arrows-alt-icon";
Expand Down Expand Up @@ -367,7 +368,11 @@ const DeviceActions = ({ device, isExtendedPartition, level, setUnappliedActions
<Flex spaceItems={{ default: "spaceItemsXs" }} className="reclaim-actions">
<DeviceActionShrink {...deviceActionProps} />
<DeviceActionDelete {...deviceActionProps} />
{hasUnappliedActions && <Button variant="plain" icon={<UndoIcon />} onClick={onUndo} aria-label={_("undo")} />}
{hasUnappliedActions && (
<Tooltip content={_("Undo last action")}>
<Button variant="plain" icon={<UndoIcon />} onClick={onUndo} aria-label={_("undo")} />
</Tooltip>
)}
</Flex>
);
};
Expand All @@ -385,13 +390,15 @@ const DeviceActionDelete = ({ device, hasBeenRemoved, newDeviceSize, onAction })
// Do not show 'delete' text for disks directly, we show 'delete' text for the contained partitions
const deleteText = device.type.v !== "disk" ? <DeleteText /> : "";
const deleteButton = (
<Button
aria-label={_("delete")}
icon={<TrashIcon />}
isAriaDisabled={isRemoveDisabled}
onClick={onRemove}
variant="plain"
/>
<Tooltip content={_("Delete partition")}>
<Button
aria-label={_("delete")}
icon={<TrashIcon />}
isAriaDisabled={isRemoveDisabled}
onClick={onRemove}
variant="plain"
/>
</Tooltip>
);

if (newDeviceSize !== undefined) {
Expand Down Expand Up @@ -447,6 +454,9 @@ const DeviceActionShrink = ({ device, hasBeenRemoved, newDeviceSize, onAction })
};

const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

const shrinkButtonRef = useRef(null);
const shrinkButtonTooltipId = idPrefix + "-shrink-tooltip-" + device["device-id"].v;
const [isPopoverOpen, setIsPopoverOpen] = useState(false);
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];
Expand All @@ -457,66 +467,86 @@ const ShrinkPopover = ({ device, isAriaDisabled, onShrink }) => {
// Therefore let's use a seperate TextInput component for the input value
const normalizedValue = inputValue.toString().replace(",", ".");

const shrinkButton = <Button variant="plain" isDisabled={isAriaDisabled} icon={<CompressArrowsAltIcon />} aria-label={_("shrink")} />;
const shrinkButton = (
<Button
ref={shrinkButtonRef}
variant="plain"
isAriaDisabled={isAriaDisabled}
icon={<CompressArrowsAltIcon />}
aria-label={_("shrink")}
aria-describedby={shrinkButtonTooltipId}
/>
);

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>
<>
<Popover
aria-label={_("shrink")}
id={idPrefix + "-shrink"}
hasAutoWidth
isVisible={isPopoverOpen}
onHide={() => setIsPopoverOpen(false)}
shouldOpen={(_event, show) => { if (!isAriaDisabled) { setIsPopoverOpen(true); show(_event, true) } }}
shouldClose={(_event, hide) => { setIsPopoverOpen(false); hide(_event) }}
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>
<Tooltip
id={shrinkButtonTooltipId}
content={_("Resize partition")}
triggerRef={shrinkButtonRef}
/>
</>
);
};

Expand Down
7 changes: 2 additions & 5 deletions test/helpers/storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,14 +537,11 @@ def reclaim_remove_device(self, device):
self.browser.click(f"#reclaim-space-modal-table tr:contains('{device}') button[aria-label='delete']")

def reclaim_check_action_button_present(self, device, action, present=True, disabled=False):
if action == "shrink":
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}"
Comment on lines +540 to +544
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

if present:
Expand Down
Loading