From ee1bae79a779d83bcff9d6f41cab72091e277721 Mon Sep 17 00:00:00 2001 From: Nagender Rao <125825782+nagu165@users.noreply.github.com> Date: Fri, 19 Jan 2024 18:36:29 +0530 Subject: [PATCH] [web] Storage: mount point selector shown only if use can change it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem : When editing a file system, the form shows a disabled selector for the mount point field. Using a disabled widget does not make sense in this case because the mount point will never be editable. The disabled widget should be replaced by plain text. Moreover, the selector currently uses a FormSelect component. It should be replaced by the Select component, similar to the file system type selector. Solution : - Updated the MountPointFormSelector to use Select component Instead of the previous FormSelect component. - The select component adds more customizability and some advanced features as well. - Updated the mountpoint field to show the mountpath when currentVolume is defined and prompts the user to choose the mountpoint from the list of volume templates in case the currentVolume is undefined. Testing : Tested manually Screenshots: Before: ![Screenshot 2024-01-09 232254](https://github.com/openSUSE/agama/assets/125825782/a68fb82c-977a-447d-a54a-f2b673eb7da1) ![Screenshot 2024-01-09 232710](https://github.com/openSUSE/agama/assets/125825782/a8960c3e-a437-4e03-946f-d8704da13a63) After: ![image](https://github.com/openSUSE/agama/assets/125825782/21680bfa-da97-4e93-a0b2-692fcf42f71b) ![image](https://github.com/openSUSE/agama/assets/125825782/17ed2255-1ec9-4267-a0bd-6ef531059b81) --------- Co-authored-by: David Díaz González --- web/package/cockpit-agama.changes | 6 ++ .../storage/ProposalVolumes.test.jsx | 2 - web/src/components/storage/VolumeForm.jsx | 86 +++++++++++++++---- .../components/storage/VolumeForm.test.jsx | 48 ++++++----- 4 files changed, 104 insertions(+), 38 deletions(-) diff --git a/web/package/cockpit-agama.changes b/web/package/cockpit-agama.changes index 130068a264..17696a219b 100644 --- a/web/package/cockpit-agama.changes +++ b/web/package/cockpit-agama.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Fri Jan 19 09:34:26 UTC 2024 - Nagu + +- Storage UI: show mount point selector only when the user can change it. + (gh#openSUSE/agama#1007) + ------------------------------------------------------------------- Thu Jan 18 08:33:52 UTC 2024 - Ancor Gonzalez Sosa diff --git a/web/src/components/storage/ProposalVolumes.test.jsx b/web/src/components/storage/ProposalVolumes.test.jsx index 5fe0f29330..68197b189e 100644 --- a/web/src/components/storage/ProposalVolumes.test.jsx +++ b/web/src/components/storage/ProposalVolumes.test.jsx @@ -215,8 +215,6 @@ describe("if there are volumes", () => { const popup = await screen.findByRole("dialog"); within(popup).getByText("Edit file system"); - const mountPointSelector = within(popup).getByRole("combobox", { name: "Mount point" }); - expect(mountPointSelector).toHaveAttribute("disabled"); }); describe("and there is transactional Btrfs volume", () => { diff --git a/web/src/components/storage/VolumeForm.jsx b/web/src/components/storage/VolumeForm.jsx index 306b6bc8d9..f593863323 100644 --- a/web/src/components/storage/VolumeForm.jsx +++ b/web/src/components/storage/VolumeForm.jsx @@ -71,15 +71,55 @@ const SizeUnitFormSelect = ({ units, ...formSelectProps }) => { * Based on {@link PF/FormSelect https://www.patternfly.org/components/forms/form-select} * * @param {object} props + * @param {string} props.value - mountPath of current selected volume * @param {Array} props.volumes - a collection of storage volumes - * @param {object} props.formSelectProps - @see {@link https://www.patternfly.org/components/forms/form-select#props} + * @param {onChangeFn} props.onChange - callback for notifying input changes + * @param {object} props.selectProps - other props sent to {@link https://www.patternfly.org/components/menus/select#props PF/Select} * @returns {ReactComponent} - */ -const MountPointFormSelect = ({ volumes, ...formSelectProps }) => { +*/ + +const MountPointFormSelect = ({ value, volumes, onChange, ...selectProps }) => { + const [isOpen, setIsOpen] = useState(false); + + const onSelect = (_, mountPath) => { + setIsOpen(false); + onChange(mountPath); + }; + + const onToggleClick = () => { + setIsOpen(!isOpen); + }; + + const toggle = toggleRef => { + return ( + + {value || _("Select a value")} + + ); + }; return ( - - { volumes.map(v => ) } - + ); }; @@ -676,7 +716,7 @@ const reducer = (state, action) => { export default function VolumeForm({ id, volume: currentVolume, templates = [], onSubmit }) { const [state, dispatch] = useReducer(reducer, currentVolume || templates[0], createInitialState); - const changeVolume = (_, mountPath) => { + const changeVolume = (mountPath) => { const volume = templates.find(t => t.mountPath === mountPath); dispatch({ type: "CHANGE_VOLUME", payload: { volume } }); }; @@ -722,17 +762,31 @@ export default function VolumeForm({ id, volume: currentVolume, templates = [], const { fsType, snapshots } = state.formData; + const ShowMountPointSelector = () => ( + + ); + + const ShowMountPoint = () =>

{state.formData.mountPoint}

; + return (
- - - + + + + } + else={ + + + + } + /> { props = { templates: [volumes.root, volumes.home, volumes.swap] }; }); -it("renders a control for displaying/selecting the mount point", () => { - plainRender(); - - screen.getByRole("combobox", { name: "Mount point" }); -}); - it("renders a control for displaying/selecting the file system type", async () => { const { user } = plainRender(); @@ -113,9 +107,10 @@ it("renders a control for displaying/selecting the file system type", async () = it("does not render the file system control if there is only one option", async () => { const { user } = plainRender(); - const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" }); - const swap = within(mountPointSelector).getByRole("option", { name: "swap" }); - await user.selectOptions(mountPointSelector, swap); + const mountPointButton = screen.getByRole("button", { name: "Mount point" }); + await user.click(mountPointButton); + const swap = screen.getByRole("option", { name: "swap" }); + await user.click(swap); await screen.findByText("Swap"); await waitFor(() => ( expect(screen.queryByRole("button", { name: "File system type" })).not.toBeInTheDocument()) @@ -151,12 +146,13 @@ it("renders the 'Auto' size option only when a volume with 'adaptive sizes' is s // that it's configured for allowing adaptive sizes too. screen.getByRole("radio", { name: "Auto" }); - const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" }); - const homeVolumeOption = screen.getByRole("option", { name: "/home" }); + const mountPointButton = screen.getByRole("button", { name: "Mount point" }); + await user.click(mountPointButton); // And we know that /home volume is not set to allow adaptive sizes. Thus, // let's select it. - await user.selectOptions(mountPointSelector, homeVolumeOption); + const homeVolumeOption = screen.getByRole("option", { name: "/home" }); + await user.click(homeVolumeOption); const autoSizeOption = screen.queryByRole("radio", { name: "Auto" }); expect(autoSizeOption).toBeNull(); @@ -296,19 +292,31 @@ describe("size validations", () => { describe("when editing a new volume", () => { beforeEach(() => { props.volume = volumes.root }); - it("renders the mount point selector as disabled", () => { + it("renders the mount point", () => { plainRender(); - const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" }); - expect(mountPointSelector).toBeDisabled(); + screen.getByText("/"); }); -}); -describe("when adding a new volume", () => { - it("renders the mount point selector as enabled", () => { + it("does not render a control for changing the mount point", async () => { plainRender(); + await waitFor(() => ( + expect(screen.queryByRole("button", { name: "Mount point" })).not.toBeInTheDocument() + )); + }); +}); - const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" }); - expect(mountPointSelector).toBeEnabled(); +describe("when adding a new volume", () => { + it("renders a control for setting the mount point", async () => { + const { user } = plainRender(); + + let mountPointButton = screen.getByRole("button", { name: "Mount point" }); + await user.click(mountPointButton); + screen.getByRole("option", { name: "/" }); + screen.getByRole("option", { name: "swap" }); + const homeMountPoint = screen.getByRole("option", { name: "/home" }); + await user.click(homeMountPoint); + mountPointButton = screen.getByRole("button", { name: "Mount point" }); + await within(mountPointButton).findByText("/home"); }); });