From 3cb9afae09fecafdfce8240c037fe081c7792212 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 13 Dec 2023 12:39:38 +0000 Subject: [PATCH 01/25] [web] Make possible opening the Sidebar via prop In the context of improving the core/Page component and how the Agama layout is handled, it's needed to have a way of opening the Sidebar from outside instead of keeping it responsible of rendering a button for such an action. Using a prop to do so following the recomendation found in the "Pitfall" warning of "Exposing your own imperaive methods" section in the React#useImpreativeHandle documentation. See [1] Please, note that this commit also drops the logic for displaying a "notification mark" because there is already a work in progress for getting ride of such a feature once we have decided that the sidebar is for placing Agama stuff only. See [2] [1] https://react.dev/reference/react/useImperativeHandle#exposing-your-own-imperative-methods [2] https://github.com/openSUSE/agama/pull/886 --- web/src/components/core/Sidebar.jsx | 118 ++++++------- web/src/components/core/Sidebar.test.jsx | 211 ++++++++++------------- 2 files changed, 142 insertions(+), 187 deletions(-) diff --git a/web/src/components/core/Sidebar.jsx b/web/src/components/core/Sidebar.jsx index 4c385ff7e2..921b474ea5 100644 --- a/web/src/components/core/Sidebar.jsx +++ b/web/src/components/core/Sidebar.jsx @@ -19,30 +19,30 @@ * find current contact information at www.suse.com. */ -import React, { useCallback, useEffect, useLayoutEffect, useRef, useState } from "react"; +import React, { useCallback, useEffect, useLayoutEffect, useRef } from "react"; import { Button, Text } from "@patternfly/react-core"; -import { Icon, AppActions } from "~/components/layout"; -import { If, NotificationMark } from "~/components/core"; -import { useNotification } from "~/context/notification"; -import useNodeSiblings from "~/hooks/useNodeSiblings"; +import { Icon } from "~/components/layout"; +import { noop } from "~/utils"; import { _ } from "~/i18n"; +import useNodeSiblings from "~/hooks/useNodeSiblings"; /** - * Agama sidebar navigation + * The Agama sidebar. * @component * + * A component intended to be the place where put things exclusively related to + * the Agama installer itself. + * * @param {object} props * @param {React.ReactElement} props.children */ -export default function Sidebar ({ children }) { - const [isOpen, setIsOpen] = useState(false); +export default function Sidebar ({ children, isOpen, onClose = noop }) { const asideRef = useRef(null); const closeButtonRef = useRef(null); - const [notification] = useNotification(); const [addAttribute, removeAttribute] = useNodeSiblings(asideRef.current); /** - * Set siblings as not interactive and not discoverable + * Set siblings as not interactive and not discoverable. */ const makeSiblingsInert = useCallback(() => { addAttribute('inert', ''); @@ -50,24 +50,23 @@ export default function Sidebar ({ children }) { }, [addAttribute]); /** - * Set siblings as interactive and discoverable + * Set siblings as interactive and discoverable. */ const makeSiblingsAlive = useCallback(() => { removeAttribute('inert'); removeAttribute('aria-hidden'); }, [removeAttribute]); - const open = () => { - setIsOpen(true); - }; - + /** + * Triggers the onClose callback. + */ const close = () => { - setIsOpen(false); + onClose(); }; /** - * Handler for automatically closing the sidebar when a click bubbles from a - * children of its content. + * Handler for automatically triggering the close function when a click bubbles from a + * sidebar children. * * @param {MouseEvent} event */ @@ -82,6 +81,7 @@ export default function Sidebar ({ children }) { }; useEffect(() => { + makeSiblingsInert(); if (isOpen) { closeButtonRef.current.focus(); makeSiblingsInert(); @@ -127,59 +127,41 @@ export default function Sidebar ({ children }) { } return ( - <> - + ); } diff --git a/web/src/components/core/Sidebar.test.jsx b/web/src/components/core/Sidebar.test.jsx index e7d255999e..12875009ee 100644 --- a/web/src/components/core/Sidebar.test.jsx +++ b/web/src/components/core/Sidebar.test.jsx @@ -21,198 +21,171 @@ import React from "react"; import { screen, within } from "@testing-library/react"; -import { installerRender, withNotificationProvider } from "~/test-utils"; +import { plainRender } from "~/test-utils"; import { If, Sidebar } from "~/components/core"; -import { createClient } from "~/client"; -// Mock some components using contexts and not relevant for below tests -jest.mock("~/components/core/LogsButton", () => () =>
LogsButton Mock
); +it("renders the sidebar hidden if isOpen prop is not given", () => { + plainRender(); -let mockIssues; - -jest.mock("~/client"); - -beforeEach(() => { - mockIssues = []; - - createClient.mockImplementation(() => { - return { - issues: jest.fn().mockResolvedValue(mockIssues), - onIssuesChange: jest.fn() - }; - }); -}); - -it("renders the sidebar initially hidden", async () => { - installerRender(withNotificationProvider()); - - const nav = await screen.findByRole("complementary", { name: /options/i }); + const nav = screen.getByRole("complementary", { name: /options/i }); expect(nav).toHaveAttribute("data-state", "hidden"); }); -it("renders a link for displaying the sidebar", async () => { - const { user } = installerRender(withNotificationProvider()); +it("renders the sidebar hidden if isOpen prop is false", () => { + plainRender(); - const link = await screen.findByLabelText(/Show/i); - const sidebar = await screen.findByRole("complementary", { name: /options/i }); - - expect(sidebar).toHaveAttribute("data-state", "hidden"); - await user.click(link); - expect(sidebar).toHaveAttribute("data-state", "visible"); + const nav = screen.getByRole("complementary", { name: /options/i }); + expect(nav).toHaveAttribute("data-state", "hidden"); }); -it("renders a link for hiding the sidebar", async () => { - const { user } = installerRender(withNotificationProvider()); - - const openLink = await screen.findByLabelText(/Show/i); - const closeLink = await screen.findByLabelText(/Hide/i); - - const sidebar = await screen.findByRole("complementary", { name: /options/i }); - - await user.click(openLink); - expect(sidebar).toHaveAttribute("data-state", "visible"); - await user.click(closeLink); - expect(sidebar).toHaveAttribute("data-state", "hidden"); -}); +describe("when isOpen prop is given", () => { + it("renders the sidebar visible", () => { + plainRender(); -it("moves the focus to the close action after opening it", async () => { - const { user } = installerRender(withNotificationProvider()); + const nav = screen.getByRole("complementary", { name: /options/i }); + expect(nav).toHaveAttribute("data-state", "visible"); + }); - const openLink = await screen.findByLabelText(/Show/i); - const closeLink = await screen.findByLabelText(/Hide/i); + it("moves the focus to the close action", () => { + plainRender(); + const closeLink = screen.getByLabelText(/Hide/i); + expect(closeLink).toHaveFocus(); + }); - expect(closeLink).not.toHaveFocus(); - await user.click(openLink); - expect(closeLink).toHaveFocus(); + it("renders a link intended for closing it that triggers the onClose callback", async () => { + const onClose = jest.fn(); + const { user } = plainRender(); + const closeLink = screen.getByLabelText(/Hide/i); + await user.click(closeLink); + expect(onClose).toHaveBeenCalled(); + }); }); +// NOTE: maybe it's time to kill this feature of keeping the sidebar open describe("onClick bubbling", () => { - it("hides the sidebar only if the user clicked on a link or button w/o keepSidebarOpen attribute", async () => { - const { user } = installerRender( - withNotificationProvider( - - Goes somewhere - Keep it open! - - - - ) + it("triggers onClose callback only if the user clicked on a link or button w/o keepSidebarOpen attribute", async () => { + const onClose = jest.fn(); + const { user } = plainRender( + + Goes somewhere + Keep it open! + + + ); - const openLink = screen.getByLabelText(/Show/i); - await user.click(openLink); const sidebar = screen.getByRole("complementary", { name: /options/i }); - expect(sidebar).toHaveAttribute("data-state", "visible"); // user clicks in the sidebar body await user.click(sidebar); - expect(sidebar).toHaveAttribute("data-state", "visible"); - - // user clicks on a button set for keeping the sidebar open - const keepOpenButton = within(sidebar).getByRole("button", { name: "Keep it open!" }); - await user.click(keepOpenButton); - expect(sidebar).toHaveAttribute("data-state", "visible"); + expect(onClose).not.toHaveBeenCalled(); // user clicks a button NOT set for keeping the sidebar open const button = within(sidebar).getByRole("button", { name: "Do something" }); await user.click(button); - expect(sidebar).toHaveAttribute("data-state", "hidden"); + expect(onClose).toHaveBeenCalled(); - // open it again - await user.click(openLink); - expect(sidebar).toHaveAttribute("data-state", "visible"); + onClose.mockClear(); - // user clicks on link set for keeping the sidebar open - const keepOpenLink = within(sidebar).getByRole("link", { name: "Keep it open!" }); - await user.click(keepOpenLink); - expect(sidebar).toHaveAttribute("data-state", "visible"); + // user clicks on a button set for keeping the sidebar open + const keepOpenButton = within(sidebar).getByRole("button", { name: "Keep it open!" }); + await user.click(keepOpenButton); + expect(onClose).not.toHaveBeenCalled(); + + onClose.mockClear(); // user clicks on link NOT set for keeping the sidebar open const link = within(sidebar).getByRole("link", { name: "Goes somewhere" }); await user.click(link); - expect(sidebar).toHaveAttribute("data-state", "hidden"); - }); -}); + expect(onClose).toHaveBeenCalled(); -describe("if there are issues", () => { - beforeEach(() => { - mockIssues = { - software: [ - { - description: "software issue 1", details: "Details 1", source: "system", severity: "warn" - } - ] - }; - }); + onClose.mockClear(); - it("includes a notification mark", async () => { - installerRender(withNotificationProvider()); - const link = await screen.findByLabelText(/Show/i); - await within(link).findByRole("status", { name: /New issues/ }); - }); -}); - -describe("if there are not issues", () => { - beforeEach(() => { - mockIssues = []; - }); - - it("does not include a notification mark", async () => { - installerRender(withNotificationProvider()); - const link = await screen.findByLabelText(/Show/i); - const mark = within(link).queryByRole("status", { name: /New issues/ }); - expect(mark).toBeNull(); + // user clicks on link set for keeping the sidebar open + const keepOpenLink = within(sidebar).getByRole("link", { name: "Keep it open!" }); + await user.click(keepOpenLink); + expect(onClose).not.toHaveBeenCalled(); }); }); describe("side effects on siblings", () => { const SidebarWithSiblings = () => { - const [isSidebarMount, setIsSidebarMount] = React.useState(true); + const [sidebarOpen, setSidebarOpen] = React.useState(false); + const [sidebarMount, setSidebarMount] = React.useState(true); + + const openSidebar = () => setSidebarOpen(true); + const closeSidebar = () => setSidebarOpen(false); // NOTE: using the "data-keep-sidebar-open" to avoid triggering the #close // function before unmounting the component. const Content = () => ( - ); return ( <> +
A sidebar sibling
- } /> + } + /> ); }; it("sets siblings as inert and aria-hidden while it's open", async () => { - const { user } = installerRender(withNotificationProvider()); + const { user } = plainRender(); - const openLink = await screen.findByLabelText(/Show/i); - const closeLink = await screen.findByLabelText(/Hide/i); + const openButton = screen.getByRole("button", { name: "open the sidebar" }); + const closeLink = screen.getByLabelText(/Hide/i); const sidebarSibling = screen.getByText("A sidebar sibling"); + + expect(openButton).not.toHaveAttribute("aria-hidden"); + expect(openButton).not.toHaveAttribute("inert"); expect(sidebarSibling).not.toHaveAttribute("aria-hidden"); expect(sidebarSibling).not.toHaveAttribute("inert"); - await user.click(openLink); + + await user.click(openButton); + + expect(openButton).toHaveAttribute("aria-hidden"); + expect(openButton).toHaveAttribute("inert"); expect(sidebarSibling).toHaveAttribute("aria-hidden"); expect(sidebarSibling).toHaveAttribute("inert"); + await user.click(closeLink); + + expect(openButton).not.toHaveAttribute("aria-hidden"); + expect(openButton).not.toHaveAttribute("inert"); expect(sidebarSibling).not.toHaveAttribute("aria-hidden"); expect(sidebarSibling).not.toHaveAttribute("inert"); }); it("removes inert and aria-hidden siblings attributes if it's unmounted", async () => { - const { user } = installerRender(withNotificationProvider()); + const { user } = plainRender(); - const openLink = await screen.findByLabelText(/Show/i); - const unmountButton = await screen.getByRole("button", { name: "Unmount Sidebar" }); + const openButton = screen.getByRole("button", { name: "open the sidebar" }); const sidebarSibling = screen.getByText("A sidebar sibling"); + + expect(openButton).not.toHaveAttribute("aria-hidden"); + expect(openButton).not.toHaveAttribute("inert"); expect(sidebarSibling).not.toHaveAttribute("aria-hidden"); expect(sidebarSibling).not.toHaveAttribute("inert"); - await user.click(openLink); + + await user.click(openButton); + + expect(openButton).toHaveAttribute("aria-hidden"); + expect(openButton).toHaveAttribute("inert"); expect(sidebarSibling).toHaveAttribute("aria-hidden"); expect(sidebarSibling).toHaveAttribute("inert"); + + const unmountButton = screen.getByRole("button", { name: "Unmount Sidebar" }); await user.click(unmountButton); + + expect(openButton).not.toHaveAttribute("aria-hidden"); + expect(openButton).not.toHaveAttribute("inert"); expect(sidebarSibling).not.toHaveAttribute("aria-hidden"); expect(sidebarSibling).not.toHaveAttribute("inert"); }); From 6bb2251dfbf0cd9a0d5b755dfa0f160b6b5eec2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 13 Dec 2023 16:10:34 +0000 Subject: [PATCH 02/25] [web] Improve page and layout management Now that everything in Agama is on a page, the core/Page component should take care of layout. This can be considered a first step toward dropping both the layout/Layout component and react-teleporter dependency. In summary, the goal of stopping using react-teleporter dependency is to improve DX by writing JSX closer to final HTML. Also, stopping using React Portals behind the scene for placing content will help now that actually it's not needed to influence the layout from Page children. When the user navigates from one page to another, mounting and rendering the layout is somehow expected and shouldn't harm performance noticeable. Creating new Pages is now much easier without having to import slots for titles, icons, actions, etc. It will be sufficient to use the Page props and Page subcomponents. --- web/src/assets/styles/composition.scss | 4 +- web/src/assets/styles/layout.scss | 75 +++++++++ web/src/components/core/Page.jsx | 164 +++++++++++------- web/src/components/core/Page.test.jsx | 224 ++++++++++++++++++------- 4 files changed, 352 insertions(+), 115 deletions(-) diff --git a/web/src/assets/styles/composition.scss b/web/src/assets/styles/composition.scss index a78eaaab05..02e15bfb26 100644 --- a/web/src/assets/styles/composition.scss +++ b/web/src/assets/styles/composition.scss @@ -30,7 +30,7 @@ body > div[inert], body > div[aria-hidden="true"], -div#agama-main-wrapper[inert], -div#agama-main-wrapper[aria-hidden="true"] { +div[data-type="agama/page-layout"] > [inert], +div[data-type="agama/page-layout"] > [aria-hidden="true"] { filter: grayscale(1) blur(2px); } diff --git a/web/src/assets/styles/layout.scss b/web/src/assets/styles/layout.scss index a0243fa6dc..542fa12c6a 100644 --- a/web/src/assets/styles/layout.scss +++ b/web/src/assets/styles/layout.scss @@ -1,3 +1,78 @@ +@use "~/assets/styles/utilities.scss"; + +[data-type="agama/page-layout"] { + @extend .shadow; + display: grid; + block-size: 100dvh; + background: white; + overflow: hidden; + grid-template-columns: 1fr; + grid-template-rows: auto 1fr auto; + grid-template-areas: + "header" + "body" + "footer" + ; + + > * { + padding: var(--spacer-small); + } + + header { + @extend .bottom-shadow; + grid-area: header; + display: flex; + align-items: center; + justify-content: space-between; + background: var(--color-primary); + color: white; + fill: white; + + h1 { + display: grid; + align-items: center; + gap: var(--spacer-small); + grid-template-columns: var(--header-icon-size) 1fr; + grid-template-areas: + "icon text" + ; + + svg { + grid-area: icon; + block-size: var(--header-icon-size); + inline-size: var(--header-icon-size); + } + + span { + grid-area: text; + } + } + } + + main { + grid-area: body; + overflow: auto; + } + + footer { + grid-area: footer; + @extend .top-shadow; + display: flex; + flex-direction: row-reverse; + justify-content: space-between; + + img { + inline-size: 30%; + max-inline-size: 150px; + } + } +} + +[data-type="agama/header-actions"] { + display: flex; + gap: var(--spacer-small); +} + .wrapper { position: relative; display: grid; diff --git a/web/src/components/core/Page.jsx b/web/src/components/core/Page.jsx index a9c0a2ece2..fbe1ea7307 100644 --- a/web/src/components/core/Page.jsx +++ b/web/src/components/core/Page.jsx @@ -23,78 +23,130 @@ import React from "react"; import { useNavigate } from "react-router-dom"; import { Button } from "@patternfly/react-core"; -import { noop } from "~/utils"; -import { Icon, Title, PageIcon, MainActions } from "~/components/layout"; +import logoUrl from "~/assets/suse-horizontal-logo.svg"; + +import { _ } from "~/i18n"; +import { partition } from "~/utils"; +import { Icon } from "~/components/layout"; +import { If } from "~/components/core"; + +/** + * Wrapper component for holding Page actions + * + * Useful and required for placing the components to be used as Page actions, usually a + * Page.Action or PF/Button + * + * @see Page examples. + * + * @param {React.ReactNode} [props.children] - a collection of Action components + */ +const Actions = ({ children }) => <>{children}; + +/** + * A convenient component representing a Page action + * + * Built on top of {@link https://www.patternfly.org/components/button PF/Button} + * + * @see Page examples. + * + * @param {React.ReactNode} props.children - content of the action + * @param {object} [props] - PF/Button props, see {@link https://www.patternfly.org/components/button#props} + */ +const Action = ({ children, navigateTo, onClick, ...props }) => { + const navigate = useNavigate(); + + props.onClick = () => { + if (typeof onClick === "function") onClick(); + if (navigateTo) navigate(navigateTo); + }; + + if (!props.size) props.size = "lg"; + + return ; +}; + +/** + * Simple action for navigating back + * + * @note it will be used by default if a page is mounted without actions + * + * TODO: Explain below note better + * @note that we cannot use navigate("..") because our routes are all nested in + * the root. + * + * @param {React.ReactNode} props.children - content of the action + * @param {object} [props] - {@link Action} props + */ +const BackAction = () => { + return ( + history.back()}> + {_("Back")} + + ); +}; /** * Displays an installation page * @component * * @example Simple usage - * + * * * * - * @example Using custom action label and callback - * console.log("Doing it...")} - * > + * @example Using a custom actions + * * - * * - * @example Using a component for the primary action - * console.log("Doing it...")}>Do it!} - * > - * + * + * alert("Are you sure?")}> + * Reset to defaults + * + * Accept + * * * * @param {object} props - * @param {string} props.icon - The icon for the page - * @param {string} props.title - The title for the page - * @param {string} [props.navigateTo="/"] - The path where the page will go after when user clicks on accept - * @param {JSX.Element} [props.action] - an element used as primary action. If present, actionLabel and actionCallback does not make effect - * @param {string} [props.actionLabel="Accept"] - The label for the primary page action - * @param {string} [props.actionVariant="primary"] - The PF/Button variant for the page action - * @param {function} [props.actionCallback=noop] - A callback to be execute when triggering the primary action - * @param {JSX.Element} [props.children] - the section content + * @param {string} [props.icon] - The icon for the page. + * @param {string} [props.title="Agama"] - The title for the page. + * @param {JSX.Element} [props.children] - The page content. */ -export default function Page({ - icon, - title, - navigateTo = "/", - action, - actionLabel = "Accept", - actionVariant = "primary", - actionCallback = noop, - children -}) { - const navigate = useNavigate(); +const Page = ({ icon, title = _("Agama"), children }) => { + // NOTE: hot reloading could make weird things when working with this + // component because the type check. + // + // @see https://stackoverflow.com/questions/55729582/check-type-of-react-component + const [actions, content] = partition(React.Children.toArray(children), child => child.type === Actions); - const pageAction = () => { - actionCallback(); - navigate(navigateTo); - }; + if (actions.length === 0) { + actions.push(); + } return ( - <> - { title && {title} } - { icon && } - - { action || - } - - {children} - +
+
+

+ } /> + {title} +

+
+ +
+ { content } +
+ +
+
+ { actions } +
+ Logo of SUSE +
+
); -} +}; + +Page.Actions = Actions; +Page.Action = Action; +Page.BackAction = BackAction; + +export default Page; diff --git a/web/src/components/core/Page.test.jsx b/web/src/components/core/Page.test.jsx index 307c17f521..5b944d5107 100644 --- a/web/src/components/core/Page.test.jsx +++ b/web/src/components/core/Page.test.jsx @@ -21,79 +21,189 @@ import React from "react"; import { screen } from "@testing-library/react"; -import { installerRender } from "~/test-utils"; +import { installerRender, plainRender, mockNavigateFn } from "~/test-utils"; import { Page } from "~/components/core"; describe("Page", () => { + beforeAll(() => { + jest.spyOn(console, "error").mockImplementation(); + }); + + afterAll(() => { + console.error.mockRestore(); + }); + it("renders given title", () => { - installerRender(); - screen.getByText("The Title"); + installerRender(, { withL10n: true }); + screen.getByRole("heading", { name: "The Title" }); + }); + + it("renders 'Agama' as title if no title is given", () => { + installerRender(, { withL10n: true }); + screen.getByRole("heading", { name: "Agama" }); + }); + + it("renders an icon if valid icon name is given", () => { + installerRender(, { withL10n: true }); + const heading = screen.getByRole("heading", { level: 1 }); + const icon = heading.querySelector("svg"); + expect(icon).toHaveAttribute("data-icon-name", "settings"); + }); + + it("does not render an icon if icon name not given", () => { + installerRender(, { withL10n: true }); + const heading = screen.getByRole("heading", { level: 1 }); + const icon = heading.querySelector("svg"); + expect(icon).toBeNull(); + // Check that component was not mounted with 'undefined' + expect(console.error).not.toHaveBeenCalled(); + }); + + it("does not render an icon if not valid icon name is given", () => { + installerRender(, { withL10n: true }); + const heading = screen.getByRole("heading", { level: 1 }); + const icon = heading.querySelector("svg"); + expect(icon).toBeNull(); + }); + + it("renders given content", () => { + installerRender( + +
Page content
+ , + { withL10n: true } + ); + + screen.getByText("Page content"); + }); + + it("renders given actions", () => { + installerRender( + + + Save + Discard + + , + { withL10n: true } + ); + + screen.getByRole("button", { name: "Save" }); + screen.getByRole("button", { name: "Discard" }); + }); + + it("renders the default 'Back' action if no actions are given", () => { + installerRender(, { withL10n: true }); + screen.getByRole("button", { name: "Back" }); }); +}); + +describe("Page.Actions", () => { + it("renders its children", () => { + plainRender( + + + + ); + + screen.getByRole("button", { name: "Plain action" }); + }); +}); - it("renders an svg if icon is given", () => { - const { container } = installerRender(); - const svgElement = container.querySelector('svg'); - expect(svgElement).not.toBeNull(); +describe("Page.Action", () => { + it("renders a button with given content", () => { + plainRender(Save); + screen.getByRole("button", { name: "Save" }); }); - it("does not render an svg if icon is not given", () => { - const { container } = installerRender(); - const svgElement = container.querySelector('svg'); - expect(svgElement).toBeNull(); + it("renders an 'lg' button when size prop is not given", () => { + plainRender(Cancel); + const button = screen.getByRole("button", { name: "Cancel" }); + expect(button.classList.contains("pf-m-display-lg")).toBe(true); }); - describe("when action node is given", () => { - it("renders the given action", () => { - installerRender(Fake action} />); - screen.getByText("Fake action"); + describe("when user clicks on it", () => { + it("triggers given onClick handler, if valid", async () => { + const onClick = jest.fn(); + const { user } = plainRender(Cancel); + const button = screen.getByRole("button", { name: "Cancel" }); + await user.click(button); + expect(onClick).toHaveBeenCalled(); }); - it("ignores action params", async () => { - const callbackFn = jest.fn(); - const { user } = installerRender( - Fake action} - actionCallback={callbackFn} - actionLabel="Great action" - /> - ); - const action = await screen.queryByText("Fake action"); - const defaultAction = await screen.queryByRole("button", { name: "Great action" }); - expect(defaultAction).toBeNull(); - await user.click(action); - expect(callbackFn).not.toHaveBeenCalled(); + it("navigates to the path given through 'navigateTo' prop", async () => { + const { user } = plainRender(Cancel); + const button = screen.getByRole("button", { name: "Cancel" }); + await user.click(button); + expect(mockNavigateFn).toHaveBeenCalledWith("/somewhere"); }); - }); - describe("when action node is not given", () => { - describe("and none action param is given", () => { - it("renders the 'default' action", () => { - installerRender(); - screen.getByRole("button", { name: "Accept" }); - }); + it("triggers form submission if it's a submit action and has an associated form", async () => { + // NOTE: using preventDefault here to avoid a jsdom error + // Error: Not implemented: HTMLFormElement.prototype.requestSubmit + const onSubmit = jest.fn((e) => { e.preventDefault() }); + + const { user } = plainRender( + <> +
+ + Send + + + ); + const button = screen.getByRole("button", { name: "Send" }); + await user.click(button); + expect(onSubmit).toHaveBeenCalled(); }); - describe("but action param are given", () => { - it("does not render the 'default' action", async () => { - installerRender(); - const defaultAction = await screen.queryByRole("button", { name: "Accept" }); - expect(defaultAction).toBeNull(); - }); - - it("renders the action according to given params", async () => { - installerRender(); - const defaultAction = await screen.findByRole("button", { name: "Ok" }); - expect(defaultAction.classList.contains("pf-m-plain")).toBe(true); - }); - - it("renders triggers given callback when user clicks the action", async () => { - const callbackFn = jest.fn(); - const { user } = installerRender(); - const defaultAction = await screen.findByRole("button", { name: "Ok" }); - await user.click(defaultAction); - - expect(callbackFn).toHaveBeenCalled(); - }); + it("triggers form submission even when onClick and navigateTo are given", async () => { + const onClick = jest.fn(); + // NOTE: using preventDefault here to avoid a jsdom error + // Error: Not implemented: HTMLFormElement.prototype.requestSubmit + const onSubmit = jest.fn((e) => { e.preventDefault() }); + + const { user } = plainRender( + <> + + + Send + + + ); + const button = screen.getByRole("button", { name: "Send" }); + await user.click(button); + expect(onSubmit).toHaveBeenCalled(); + expect(onClick).toHaveBeenCalled(); + expect(mockNavigateFn).toHaveBeenCalledWith("/somewhere"); }); }); }); + +describe("Page.BackAction", () => { + beforeAll(() => { + jest.spyOn(history, "back").mockImplementation(); + }); + + afterAll(() => { + history.back.mockRestore(); + }); + + it("renders a 'Back' button with large size and secondary style", () => { + plainRender(); + const button = screen.getByRole("button", { name: "Back" }); + expect(button.classList.contains("pf-m-display-lg")).toBe(true); + expect(button.classList.contains("pf-m-secondary")).toBe(true); + }); + + it("triggers history.back() when user clicks on it", async () => { + const { user } = plainRender(); + const button = screen.getByRole("button", { name: "Back" }); + await user.click(button); + expect(history.back).toHaveBeenCalled(); + }); +}); From 386e5163bfa6ce2cc06268c9be08358580eec64c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 13 Dec 2023 21:41:38 +0000 Subject: [PATCH 03/25] [web] Mount Sidebar from Page component Because it does not make sense to continue mounting it from the App.jsx. Instead, core/Page is the right place now that the layout is rendered from there. It still being a sibling of the rest of the content. --- web/src/App.jsx | 45 ++++------------------- web/src/App.test.jsx | 2 +- web/src/components/core/Page.jsx | 52 +++++++++++++++++++++++++-- web/src/components/core/Page.test.jsx | 9 +++++ 4 files changed, 67 insertions(+), 41 deletions(-) diff --git a/web/src/App.jsx b/web/src/App.jsx index c374c3c87b..08daa5a217 100644 --- a/web/src/App.jsx +++ b/web/src/App.jsx @@ -22,24 +22,12 @@ import React, { useEffect, useState } from "react"; import { Outlet } from "react-router-dom"; -import { _ } from "~/i18n"; import { useInstallerClient, useInstallerClientStatus } from "~/context/installer"; import { useProduct } from "./context/product"; import { STARTUP, INSTALL } from "~/client/phase"; import { BUSY } from "~/client/status"; -import { - About, - DBusError, - Disclosure, - Installation, - IssuesLink, - LogsButton, - ShowLogButton, - ShowTerminalButton, - Sidebar -} from "~/components/core"; -import { LanguageSwitcher } from "./components/l10n"; +import { DBusError, Installation } from "~/components/core"; import { Layout, Loading, Title } from "./components/layout"; import { useInstallerL10n } from "./context/installerL10n"; @@ -103,31 +91,12 @@ function App() { }; return ( - <> - -
- - - - - - - -
-
-
- -
-
- - - - {/* this is the name of the tool, do not translate it */} - {/* eslint-disable-next-line i18next/no-literal-string */} - Agama - {language && } - - + + {/* this is the name of the tool, do not translate it */} + {/* eslint-disable-next-line i18next/no-literal-string */} + Agama + {language && } + ); } diff --git a/web/src/App.test.jsx b/web/src/App.test.jsx index 0e6bab251d..71784079fc 100644 --- a/web/src/App.test.jsx +++ b/web/src/App.test.jsx @@ -22,6 +22,7 @@ import React from "react"; import { act, screen } from "@testing-library/react"; import { installerRender } from "~/test-utils"; + import App from "./App"; import { createClient } from "~/client"; import { STARTUP, CONFIG, INSTALL } from "~/client/phase"; @@ -46,7 +47,6 @@ jest.mock("~/context/product", () => ({ jest.mock("~/components/core/DBusError", () =>
D-BusError Mock
); jest.mock("~/components/questions/Questions", () => () =>
Questions Mock
); jest.mock("~/components/core/Installation", () => () =>
Installation Mock
); -jest.mock("~/components/core/Sidebar", () => () =>
Sidebar Mock
); // this object holds the mocked callbacks const callbacks = {}; diff --git a/web/src/components/core/Page.jsx b/web/src/components/core/Page.jsx index fbe1ea7307..9824241f5b 100644 --- a/web/src/components/core/Page.jsx +++ b/web/src/components/core/Page.jsx @@ -19,7 +19,7 @@ * find current contact information at www.suse.com. */ -import React from "react"; +import React, { useState } from "react"; import { useNavigate } from "react-router-dom"; import { Button } from "@patternfly/react-core"; @@ -28,7 +28,17 @@ import logoUrl from "~/assets/suse-horizontal-logo.svg"; import { _ } from "~/i18n"; import { partition } from "~/utils"; import { Icon } from "~/components/layout"; -import { If } from "~/components/core"; +import { LanguageSwitcher } from "~/components/l10n"; +import { + About, + Disclosure, + If, + IssuesLink, + LogsButton, + ShowLogButton, + ShowTerminalButton, + Sidebar +} from "~/components/core"; /** * Wrapper component for holding Page actions @@ -89,6 +99,11 @@ const BackAction = () => { * Displays an installation page * @component * + * FIXME: Improve the note below + * @note Sidebar must be mounted as sibling of the page content to make it work + * as expected (allow it to be hidden, making inert siblings when open, etc). + * Remember that Sidebar is going to content only things related to Agama itself + * * @example Simple usage * * @@ -112,6 +127,8 @@ const BackAction = () => { * @param {JSX.Element} [props.children] - The page content. */ const Page = ({ icon, title = _("Agama"), children }) => { + const [sidebarOpen, setSidebarOpen] = useState(false); + // NOTE: hot reloading could make weird things when working with this // component because the type check. // @@ -122,6 +139,9 @@ const Page = ({ icon, title = _("Agama"), children }) => { actions.push(); } + const openSidebar = () => setSidebarOpen(true); + const closeSidebar = () => setSidebarOpen(false); + return (
@@ -129,6 +149,17 @@ const Page = ({ icon, title = _("Agama"), children }) => { } /> {title} +
+ +
@@ -141,6 +172,23 @@ const Page = ({ icon, title = _("Agama"), children }) => {
Logo of SUSE + + +
+ + + + + + + +
+
+
+ +
+
+ ); }; diff --git a/web/src/components/core/Page.test.jsx b/web/src/components/core/Page.test.jsx index 5b944d5107..4e0b97d471 100644 --- a/web/src/components/core/Page.test.jsx +++ b/web/src/components/core/Page.test.jsx @@ -96,6 +96,15 @@ describe("Page", () => { installerRender(, { withL10n: true }); screen.getByRole("button", { name: "Back" }); }); + + it("renders the Agama sidebar", async () => { + const { user } = installerRender(, { withL10n: true }); + + const openSidebarButton = screen.getByRole("button", { name: "Show global options" }); + + await user.click(openSidebarButton); + screen.getByRole("complementary", { name: /options/i }); + }); }); describe("Page.Actions", () => { From 8edaca439325554a02646596850a9c498a8a0ce8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 13 Dec 2023 21:54:55 +0000 Subject: [PATCH 04/25] [web] Do not mark name of the tool for translation --- web/src/components/core/Page.jsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/web/src/components/core/Page.jsx b/web/src/components/core/Page.jsx index 9824241f5b..f4d0847714 100644 --- a/web/src/components/core/Page.jsx +++ b/web/src/components/core/Page.jsx @@ -123,10 +123,12 @@ const BackAction = () => { * * @param {object} props * @param {string} [props.icon] - The icon for the page. - * @param {string} [props.title="Agama"] - The title for the page. + * @param {string} [props.title="Agama"] - The title for the page. By default it + * uses the name of the tool, do not mark it for translation. * @param {JSX.Element} [props.children] - The page content. + * */ -const Page = ({ icon, title = _("Agama"), children }) => { +const Page = ({ icon, title = "Agama", children }) => { const [sidebarOpen, setSidebarOpen] = useState(false); // NOTE: hot reloading could make weird things when working with this From c08c599daa50cf0d0689ae33ce47c8c78c8c487d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Thu, 14 Dec 2023 08:27:18 +0000 Subject: [PATCH 05/25] [web] Adap core/Page for rendering PageOptions too --- web/src/components/core/Page.jsx | 7 ++++++- web/src/components/core/Page.test.jsx | 23 +++++++++++++++++++++-- web/src/components/core/PageOptions.jsx | 9 ++++++++- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/web/src/components/core/Page.jsx b/web/src/components/core/Page.jsx index f4d0847714..a45e7b49b0 100644 --- a/web/src/components/core/Page.jsx +++ b/web/src/components/core/Page.jsx @@ -135,7 +135,11 @@ const Page = ({ icon, title = "Agama", children }) => { // component because the type check. // // @see https://stackoverflow.com/questions/55729582/check-type-of-react-component - const [actions, content] = partition(React.Children.toArray(children), child => child.type === Actions); + const [actions, rest] = partition(React.Children.toArray(children), child => child.type === Actions); + + // TODO: move PageOptions to an internal Page component + // TODO: Improve and document below line + const [options, content] = partition(rest, child => child.type.name?.endsWith("PageOptions")); if (actions.length === 0) { actions.push(); @@ -152,6 +156,7 @@ const Page = ({ icon, title = "Agama", children }) => { {title}
+ { options } + + + , + { withL10n: true } + ); + + const [header,] = screen.getAllByRole("banner"); + const optionsButton = within(header).getByRole("button", { name: "Show page options" }); + await user.click(optionsButton); + screen.getByRole("menuitem", { name: "Switch to advanced mode" }); + }); + it("renders given actions", () => { installerRender( diff --git a/web/src/components/core/PageOptions.jsx b/web/src/components/core/PageOptions.jsx index 9f2de813dd..87fe259dcf 100644 --- a/web/src/components/core/PageOptions.jsx +++ b/web/src/components/core/PageOptions.jsx @@ -24,6 +24,7 @@ import { Dropdown, DropdownGroup, DropdownItem, DropdownList, MenuToggle } from '@patternfly/react-core'; +import { _ } from "~/i18n"; import { Icon, PageOptions as PageOptionsSlot } from "~/components/layout"; /** @@ -35,7 +36,13 @@ import { Icon, PageOptions as PageOptionsSlot } from "~/components/layout"; */ const Toggler = ({ toggleRef, onClick }) => { return ( - + ); From 1bcf44b72018c42cf3e9937a2b74275c18019746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Thu, 14 Dec 2023 08:34:20 +0000 Subject: [PATCH 06/25] [web] Minor adjustments in core/Page.test.jsx --- web/src/components/core/Page.test.jsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/web/src/components/core/Page.test.jsx b/web/src/components/core/Page.test.jsx index fe94db308e..7dd29546d4 100644 --- a/web/src/components/core/Page.test.jsx +++ b/web/src/components/core/Page.test.jsx @@ -90,13 +90,14 @@ describe("Page", () => { { withL10n: true } ); + // Sidebar is rendering it's own header, let's ignore it const [header,] = screen.getAllByRole("banner"); const optionsButton = within(header).getByRole("button", { name: "Show page options" }); await user.click(optionsButton); screen.getByRole("menuitem", { name: "Switch to advanced mode" }); }); - it("renders given actions", () => { + it("renders found page actions in the footer", () => { installerRender( @@ -107,8 +108,10 @@ describe("Page", () => { { withL10n: true } ); - screen.getByRole("button", { name: "Save" }); - screen.getByRole("button", { name: "Discard" }); + // Sidebar is rendering it's own footer, let's ignore it + const [footer,] = screen.getAllByRole("contentinfo"); + within(footer).getByRole("button", { name: "Save" }); + within(footer).getByRole("button", { name: "Discard" }); }); it("renders the default 'Back' action if no actions are given", () => { From f97b3c2f197ecde6cc357399c3b57bc478b6fc20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Thu, 14 Dec 2023 08:51:34 +0000 Subject: [PATCH 07/25] [web] Stop using core/Layout and its slots Becuase now core/Page is the component responsible to put everything in the right place via its props or internal components. --- web/src/App.jsx | 11 +- web/src/components/core/DBusError.jsx | 59 +++--- web/src/components/core/DBusError.test.jsx | 17 +- .../components/core/InstallationFinished.jsx | 57 +++--- .../core/InstallationFinished.test.jsx | 21 +-- .../components/core/InstallationProgress.jsx | 9 +- .../core/InstallationProgress.test.jsx | 14 +- web/src/components/core/IssuesPage.jsx | 9 +- web/src/components/core/IssuesPage.test.jsx | 5 +- web/src/components/core/PageOptions.jsx | 28 ++- web/src/components/l10n/L10nPage.jsx | 9 +- web/src/components/l10n/L10nPage.test.jsx | 25 ++- web/src/components/layout/Layout.jsx | 178 ------------------ web/src/components/layout/index.js | 2 - web/src/components/network/NetworkPage.jsx | 2 +- .../components/network/NetworkPage.test.jsx | 4 + web/src/components/overview/Overview.jsx | 7 +- web/src/components/overview/Overview.test.jsx | 35 ++-- web/src/components/product/ProductPage.jsx | 3 +- .../components/product/ProductPage.test.jsx | 5 +- .../product/ProductSelectionPage.jsx | 26 ++- .../product/ProductSelectionPage.test.jsx | 20 +- .../components/questions/Questions.test.jsx | 30 ++- web/src/components/software/SoftwarePage.jsx | 2 +- .../components/software/SoftwarePage.test.jsx | 29 +-- web/src/components/storage/DASDPage.jsx | 7 - web/src/components/storage/ISCSIPage.jsx | 19 +- web/src/components/storage/ProposalPage.jsx | 2 +- .../components/storage/ProposalPage.test.jsx | 7 +- web/src/components/storage/ZFCPPage.jsx | 9 +- web/src/components/storage/ZFCPPage.test.jsx | 19 +- .../users/RootPasswordPopup.test.jsx | 6 +- .../components/users/RootSSHKeyPopup.test.jsx | 6 +- web/src/test-utils.js | 10 +- 34 files changed, 236 insertions(+), 456 deletions(-) delete mode 100644 web/src/components/layout/Layout.jsx diff --git a/web/src/App.jsx b/web/src/App.jsx index 08daa5a217..8a333563a3 100644 --- a/web/src/App.jsx +++ b/web/src/App.jsx @@ -27,8 +27,8 @@ import { useProduct } from "./context/product"; import { STARTUP, INSTALL } from "~/client/phase"; import { BUSY } from "~/client/status"; -import { DBusError, Installation } from "~/components/core"; -import { Layout, Loading, Title } from "./components/layout"; +import { DBusError, If, Installation } from "~/components/core"; +import { Loading } from "./components/layout"; import { useInstallerL10n } from "./context/installerL10n"; // D-Bus connection attempts before displaying an error. @@ -91,12 +91,7 @@ function App() { }; return ( - - {/* this is the name of the tool, do not translate it */} - {/* eslint-disable-next-line i18next/no-literal-string */} - Agama - {language && } - + } /> ); } diff --git a/web/src/components/core/DBusError.jsx b/web/src/components/core/DBusError.jsx index d2a325562e..be79b138f0 100644 --- a/web/src/components/core/DBusError.jsx +++ b/web/src/components/core/DBusError.jsx @@ -20,47 +20,38 @@ */ import React from "react"; -import { Button, EmptyState, EmptyStateIcon, EmptyStateBody, EmptyStateHeader } from "@patternfly/react-core"; -import { locationReload } from "~/utils"; +import { EmptyState, EmptyStateIcon, EmptyStateBody, EmptyStateHeader } from "@patternfly/react-core"; +import { Center, Icon } from "~/components/layout"; +import { Page } from "~/components/core"; import { _ } from "~/i18n"; - -import { - Center, - Icon, - MainActions, - PageIcon, - Title as PageTitle, -} from "~/components/layout"; +import { locationReload } from "~/utils"; const ErrorIcon = () => ; -// TODO: an example -const ReloadAction = () => ( - -); - function DBusError() { return ( -
- {/* TRANSLATORS: page title */} - {_("D-Bus Error")} - - + // TRANSLATORS: page title + +
+ + } + /> + + {_("Could not connect to the D-Bus service. Please, check whether it is running.")} + + +
- - } - /> - - {_("Could not connect to the D-Bus service. Please, check whether it is running.")} - - -
+ + locationReload()}> + {/* TRANSLATORS: button label */} + {_("Reload")} + + + ); } diff --git a/web/src/components/core/DBusError.test.jsx b/web/src/components/core/DBusError.test.jsx index ff901d9e19..ba985f1c8f 100644 --- a/web/src/components/core/DBusError.test.jsx +++ b/web/src/components/core/DBusError.test.jsx @@ -20,28 +20,27 @@ */ import React from "react"; - import { screen } from "@testing-library/react"; import { plainRender } from "~/test-utils"; -import * as utils from "~/utils"; +import * as utils from "~/utils"; import { DBusError } from "~/components/core"; +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); + describe("DBusError", () => { it("includes a generic D-Bus connection problem message", () => { plainRender(); - - expect(screen.getByText(/Could not connect to the D-Bus service/i)) - .toBeInTheDocument(); + screen.getByText(/Could not connect to the D-Bus service/i); }); it("calls location.reload when user clicks on 'Reload'", async () => { jest.spyOn(utils, "locationReload").mockImplementation(utils.noop); - - const { user } = plainRender(, { layout: true }); - + const { user } = plainRender(); const reloadButton = await screen.findByRole("button", { name: /Reload/i }); - await user.click(reloadButton); expect(utils.locationReload).toHaveBeenCalled(); }); diff --git a/web/src/components/core/InstallationFinished.jsx b/web/src/components/core/InstallationFinished.jsx index 8c492e508a..e8dcb12f67 100644 --- a/web/src/components/core/InstallationFinished.jsx +++ b/web/src/components/core/InstallationFinished.jsx @@ -21,7 +21,6 @@ import React, { useState, useEffect } from "react"; import { - Button, Text, EmptyState, EmptyStateBody, @@ -29,7 +28,8 @@ import { EmptyStateIcon, } from "@patternfly/react-core"; -import { Center, Icon, Title as SectionTitle, PageIcon, MainActions } from "~/components/layout"; +import { Page } from "~/components/core"; +import { Center, Icon } from "~/components/layout"; import { useInstallerClient } from "~/context/installer"; import { _ } from "~/i18n"; @@ -55,34 +55,33 @@ function InstallationFinished() { }); return ( -
- {_("Installation Finished")} - - - - + // TRANSLATORS: page title + +
+ + } + /> + + {_("The installation on your machine is complete.")} + + { + iguana + ? _("At this point you can power off the machine.") + : _("At this point you can reboot the machine to log in to the new system.") + } + + {_("Have a lot of fun! Your openSUSE Development Team.")} + + +
- - } - /> - - {_("The installation on your machine is complete.")} - - { - iguana - ? _("At this point you can power off the machine.") - : _("At this point you can reboot the machine to log in to the new system.") - } - - {_("Have a lot of fun! Your openSUSE Development Team.")} - - -
+ + {buttonCaption} + + ); } diff --git a/web/src/components/core/InstallationFinished.test.jsx b/web/src/components/core/InstallationFinished.test.jsx index 5c2653fc68..60e2eada8a 100644 --- a/web/src/components/core/InstallationFinished.test.jsx +++ b/web/src/components/core/InstallationFinished.test.jsx @@ -28,6 +28,10 @@ import { createClient } from "~/client"; import InstallationFinished from "./InstallationFinished"; jest.mock("~/client"); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); const finishInstallationFn = jest.fn(); @@ -38,30 +42,25 @@ describe("InstallationFinished", () => { manager: { finishInstallation: finishInstallationFn, useIguana: () => Promise.resolve(false) - }, - network: { - config: () => Promise.resolve({ addresses: [], hostname: "example.net" }) } }; }); }); - it("shows the finished installation screen", async () => { + it("shows the finished installation screen", () => { installerRender(); - - await screen.findByText("Congratulations!"); + screen.getByText("Congratulations!"); }); - it("shows a 'Reboot' button", async () => { + it("shows a 'Reboot' button", () => { installerRender(); - - await screen.findByRole("button", { name: /Reboot/i }); + screen.getByRole("button", { name: /Reboot/i }); }); it("reboots the system if the user clicks on 'Reboot' button", async () => { const { user } = installerRender(); - const button = await screen.findByRole("button", { name: /Reboot/i }); - await user.click(button); + const rebootButton = screen.getByRole("button", { name: /Reboot/i }); + await user.click(rebootButton); expect(finishInstallationFn).toHaveBeenCalled(); }); }); diff --git a/web/src/components/core/InstallationProgress.jsx b/web/src/components/core/InstallationProgress.jsx index 9acab999c3..6497748722 100644 --- a/web/src/components/core/InstallationProgress.jsx +++ b/web/src/components/core/InstallationProgress.jsx @@ -22,18 +22,17 @@ import React from "react"; import ProgressReport from "./ProgressReport"; -import { Center, Icon, Title, PageIcon } from "~/components/layout"; +import { Center } from "~/components/layout"; +import { Page } from "~/components/core"; import { Questions } from "~/components/questions"; import { _ } from "~/i18n"; function InstallationProgress() { return ( - <> - {_("Installing")} - +
- + ); } diff --git a/web/src/components/core/InstallationProgress.test.jsx b/web/src/components/core/InstallationProgress.test.jsx index 4d92442196..feeec25db0 100644 --- a/web/src/components/core/InstallationProgress.test.jsx +++ b/web/src/components/core/InstallationProgress.test.jsx @@ -27,17 +27,19 @@ import { installerRender } from "~/test-utils"; import InstallationProgress from "./InstallationProgress"; jest.mock("~/components/core/ProgressReport", () => () =>
ProgressReport Mock
); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); describe("InstallationProgress", () => { - it("uses 'Installing' as title", async () => { + it("uses 'Installing' as title", () => { installerRender(); - - await screen.findByText("Installing"); + screen.getByText("Installing"); }); - it("renders progress report", async () => { + it("renders progress report", () => { installerRender(); - - await screen.findByText("ProgressReport Mock"); + screen.getByText("ProgressReport Mock"); }); }); diff --git a/web/src/components/core/IssuesPage.jsx b/web/src/components/core/IssuesPage.jsx index f076c6aece..8a6fa853ee 100644 --- a/web/src/components/core/IssuesPage.jsx +++ b/web/src/components/core/IssuesPage.jsx @@ -172,13 +172,8 @@ export default function IssuesPage() { }, [client, load, update]); return ( - + // TRANSLATORS: page title + } diff --git a/web/src/components/core/IssuesPage.test.jsx b/web/src/components/core/IssuesPage.test.jsx index 97badf900d..8f98ed5e87 100644 --- a/web/src/components/core/IssuesPage.test.jsx +++ b/web/src/components/core/IssuesPage.test.jsx @@ -26,13 +26,16 @@ import { createClient } from "~/client"; import { IssuesPage } from "~/components/core"; jest.mock("~/client"); - jest.mock("@patternfly/react-core", () => { return { ...jest.requireActual("@patternfly/react-core"), Skeleton: () =>
PFSkeleton
}; }); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); const issues = { product: [], diff --git a/web/src/components/core/PageOptions.jsx b/web/src/components/core/PageOptions.jsx index 87fe259dcf..905a0b383f 100644 --- a/web/src/components/core/PageOptions.jsx +++ b/web/src/components/core/PageOptions.jsx @@ -25,7 +25,7 @@ import { MenuToggle } from '@patternfly/react-core'; import { _ } from "~/i18n"; -import { Icon, PageOptions as PageOptionsSlot } from "~/components/layout"; +import { Icon } from "~/components/layout"; /** * Internal component to build the {PageOptions} toggler @@ -150,20 +150,18 @@ const PageOptions = ({ children }) => { const close = () => setIsOpen(false); return ( - - } - onSelect={close} - onOpenChange={close} - popperProps={{ minWidth: "150px", position: "right" }} - className="page-options" - > - - {Array(children)} - - - + } + onSelect={close} + onOpenChange={close} + popperProps={{ minWidth: "150px", position: "right" }} + className="page-options" + > + + {Array(children)} + + ); }; diff --git a/web/src/components/l10n/L10nPage.jsx b/web/src/components/l10n/L10nPage.jsx index 4121631782..17516fbecf 100644 --- a/web/src/components/l10n/L10nPage.jsx +++ b/web/src/components/l10n/L10nPage.jsx @@ -380,13 +380,8 @@ const KeymapSection = () => { */ export default function L10nPage() { return ( - + // TRANSLATORS: page title + diff --git a/web/src/components/l10n/L10nPage.test.jsx b/web/src/components/l10n/L10nPage.test.jsx index 7423b0b284..384207349a 100644 --- a/web/src/components/l10n/L10nPage.test.jsx +++ b/web/src/components/l10n/L10nPage.test.jsx @@ -22,7 +22,7 @@ import React from "react"; import { screen, waitFor, within } from "@testing-library/react"; -import { installerRender } from "~/test-utils"; +import { installerRender, plainRender } from "~/test-utils"; import { L10nPage } from "~/components/l10n"; import { createClient } from "~/client"; @@ -76,6 +76,11 @@ createClient.mockImplementation(() => ( } )); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); + beforeEach(() => { mockL10nClient = { setLocales: jest.fn().mockResolvedValue(), @@ -89,7 +94,7 @@ beforeEach(() => { }); it("renders a section for configuring the language", () => { - installerRender(); + plainRender(); screen.getByText("Language"); }); @@ -99,7 +104,7 @@ describe("if there is no selected language", () => { }); it("renders a button for selecting a language", () => { - installerRender(); + plainRender(); screen.getByText("Language not selected yet"); screen.getByRole("button", { name: "Select language" }); }); @@ -111,7 +116,7 @@ describe("if there is a selected language", () => { }); it("renders a button for changing the language", () => { - installerRender(); + plainRender(); screen.getByText("Spanish - Spain"); screen.getByRole("button", { name: "Change language" }); }); @@ -193,7 +198,7 @@ describe("when the button for changing the language is clicked", () => { }); it("renders a section for configuring the keyboard", () => { - installerRender(); + plainRender(); screen.getByText("Keyboard"); }); @@ -203,7 +208,7 @@ describe("if there is no selected keyboard", () => { }); it("renders a button for selecting a keyboard", () => { - installerRender(); + plainRender(); screen.getByText("Keyboard not selected yet"); screen.getByRole("button", { name: "Select keyboard" }); }); @@ -215,7 +220,7 @@ describe("if there is a selected keyboard", () => { }); it("renders a button for changing the keyboard", () => { - installerRender(); + plainRender(); screen.getByText("Spanish"); screen.getByRole("button", { name: "Change keyboard" }); }); @@ -297,7 +302,7 @@ describe("when the button for changing the keyboard is clicked", () => { }); it("renders a section for configuring the time zone", () => { - installerRender(); + plainRender(); screen.getByText("Time zone"); }); @@ -307,7 +312,7 @@ describe("if there is no selected time zone", () => { }); it("renders a button for selecting a time zone", () => { - installerRender(); + plainRender(); screen.getByText("Time zone not selected yet"); screen.getByRole("button", { name: "Select time zone" }); }); @@ -319,7 +324,7 @@ describe("if there is a selected time zone", () => { }); it("renders a button for changing the time zone", () => { - installerRender(); + plainRender(); screen.getByText("Atlantic - Canary"); screen.getByRole("button", { name: "Change time zone" }); }); diff --git a/web/src/components/layout/Layout.jsx b/web/src/components/layout/Layout.jsx deleted file mode 100644 index 3fbf257f6d..0000000000 --- a/web/src/components/layout/Layout.jsx +++ /dev/null @@ -1,178 +0,0 @@ -/* - * Copyright (c) [2022-2023] SUSE LLC - * - * All Rights Reserved. - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of version 2 of the GNU General Public License as published - * by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for - * more details. - * - * You should have received a copy of the GNU General Public License along - * with this program; if not, contact SUSE LLC. - * - * To contact SUSE LLC about this file by physical or electronic mail, you may - * find current contact information at www.suse.com. - */ - -import React from "react"; - -import logoUrl from "~/assets/suse-horizontal-logo.svg"; -import { createTeleporter } from "react-teleporter"; - -const PageTitle = createTeleporter(); -const PageActions = createTeleporter(); -const HeaderActions = createTeleporter(); -const HeaderIcon = createTeleporter(); -const FooterActions = createTeleporter(); -const FooterInfoArea = createTeleporter(); - -/** - * - * The Agama main layout component. - * - * It displays the content in a single column with a fixed header and footer. - * - * To achieve a {@link https://gregberge.com/blog/react-scalable-layout scalable layout}, - * it uses {@link https://reactjs.org/docs/portals.html React Portals} through - * {@link https://github.com/gregberge/react-teleporter react-teleporter}. In other words, - * it is mounted only once and gets influenced by other components by using the created - * slots (Title, PageIcon, MainActions, etc). - * - * @example - * - * - * - * - * Dashboard - * - * - * - * - * - * - * - * - * @param {object} props - component props - * @param {React.ReactNode} [props.children] - the section content - * - */ -function Layout({ children }) { - return ( -
-
-

- - -

- -
- - -
- -
- -
- {children} -
- -
- - Logo of SUSE -
-
- ); -} - -/** - * Component for setting the title shown at the header - * - * @example - * Partitioner - */ -const Title = PageTitle.Source; - -/** - * Component for setting the icon shown at the header left - * - * @example - * import { PageIcon } from "agama-layout"; - * import { FancyIcon } from "icons-package"; - * ... - * - */ -const PageIcon = HeaderIcon.Source; - -/** - * Component for setting page actions shown on the header right - * - * @example - * import { PageActions } from "agama-layout"; - * import { FancyButton } from "somewhere"; - * ... - * - * console.log("do something")} /> - * - */ -const PageOptions = PageActions.Source; - -/** - * Component for setting global actions shown on the header right - * - * @example - * import { AppActions } from "agama-layout"; - * import { FancyButton } from "somewhere"; - * ... - * - * console.log("do something")} /> - * - */ -const AppActions = HeaderActions.Source; - -/** - * Component for setting the main actions shown on the footer right - * - * @example - * import { MainActions } from "agama-layout"; - * import { FancyButton } from "somewhere"; - * ... - * - * console.log("do something")} /> - * - */ -const MainActions = FooterActions.Source; - -/** - * Component for setting the additional content shown at the footer - * - * @example - * import { AdditionalInfo } from "agama-layout"; - * import { About, HostIp } from "somewhere"; - * - * ... - * - * - * console.log("show a pop-up with more information")} /> - * - * - */ -const AdditionalInfo = FooterInfoArea.Source; - -export { - Layout as default, - Title, - PageIcon, - AppActions, - PageOptions, - MainActions, - AdditionalInfo, -}; diff --git a/web/src/components/layout/index.js b/web/src/components/layout/index.js index 55f61a4ac7..d9e095f2fa 100644 --- a/web/src/components/layout/index.js +++ b/web/src/components/layout/index.js @@ -19,8 +19,6 @@ * find current contact information at www.suse.com. */ -export * from "./Layout"; -export { default as Layout } from "./Layout"; export { default as Icon } from "./Icon"; export { default as Center } from "./Center"; export { default as Loading } from "./Loading"; diff --git a/web/src/components/network/NetworkPage.jsx b/web/src/components/network/NetworkPage.jsx index 731ad79de5..57352c54f4 100644 --- a/web/src/components/network/NetworkPage.jsx +++ b/web/src/components/network/NetworkPage.jsx @@ -167,7 +167,7 @@ export default function NetworkPage() { return ( // TRANSLATORS: page title - + { /* TRANSLATORS: page section */ }
{ ready ? : } diff --git a/web/src/components/network/NetworkPage.test.jsx b/web/src/components/network/NetworkPage.test.jsx index 1aaa8db462..5df57fe4aa 100644 --- a/web/src/components/network/NetworkPage.test.jsx +++ b/web/src/components/network/NetworkPage.test.jsx @@ -27,6 +27,10 @@ import { ConnectionTypes } from "~/client/network"; import { createClient } from "~/client"; jest.mock("~/client"); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); const wiredConnection = { id: "wired-1", diff --git a/web/src/components/overview/Overview.jsx b/web/src/components/overview/Overview.jsx index eebce05b48..a7d60a33fb 100644 --- a/web/src/components/overview/Overview.jsx +++ b/web/src/components/overview/Overview.jsx @@ -43,10 +43,9 @@ function Overview() { return ( setShowErrors(true)} />} > @@ -54,6 +53,10 @@ function Overview() { + + + setShowErrors(true)} /> + ); } diff --git a/web/src/components/overview/Overview.test.jsx b/web/src/components/overview/Overview.test.jsx index dcb0733bd5..2a64535ffb 100644 --- a/web/src/components/overview/Overview.test.jsx +++ b/web/src/components/overview/Overview.test.jsx @@ -21,9 +21,9 @@ import React from "react"; import { screen } from "@testing-library/react"; -import { installerRender } from "~/test-utils"; -import Overview from "./Overview"; +import { plainRender } from "~/test-utils"; import { createClient } from "~/client"; +import Overview from "./Overview"; let mockProduct; const mockProducts = [ @@ -33,7 +33,6 @@ const mockProducts = [ const startInstallationFn = jest.fn(); jest.mock("~/client"); - jest.mock("~/context/product", () => ({ ...jest.requireActual("~/context/product"), useProduct: () => { @@ -43,7 +42,6 @@ jest.mock("~/context/product", () => ({ }; } })); - jest.mock("~/components/overview/ProductSection", () => () =>
Product Section
); jest.mock("~/components/overview/L10nSection", () => () =>
Localization Section
); jest.mock("~/components/overview/StorageSection", () => () =>
Storage Section
); @@ -51,6 +49,10 @@ jest.mock("~/components/overview/NetworkSection", () => () =>
Network Secti jest.mock("~/components/overview/UsersSection", () => () =>
Users Section
); jest.mock("~/components/overview/SoftwareSection", () => () =>
Software Section
); jest.mock("~/components/core/InstallButton", () => () =>
Install Button
); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); beforeEach(() => { mockProduct = { id: "openSUSE", name: "openSUSE Tumbleweed" }; @@ -67,18 +69,16 @@ beforeEach(() => { }); describe("when product is selected", () => { - it("renders the Overview and the Install button", async () => { - installerRender(); - const title = screen.getByText(/installation summary/i); - expect(title).toBeInTheDocument(); - - await screen.findByText("Product Section"); - await screen.findByText("Localization Section"); - await screen.findByText("Network Section"); - await screen.findByText("Storage Section"); - await screen.findByText("Users Section"); - await screen.findByText("Software Section"); - await screen.findByText("Install Button"); + it("renders the Overview and the Install button", () => { + plainRender(); + screen.getByRole("heading", { name: "Installation Summary", level: 1 }); + screen.getByText("Product Section"); + screen.getByText("Localization Section"); + screen.getByText("Network Section"); + screen.getByText("Storage Section"); + screen.getByText("Users Section"); + screen.getByText("Software Section"); + screen.getByText("Install Button"); }); }); @@ -88,8 +88,7 @@ describe("when no product is selected", () => { }); it("redirects to the product selection page", async () => { - installerRender(); - + plainRender(); // react-router-dom Navigate is mocked. See test-utils for more details. await screen.findByText("Navigating to /products"); }); diff --git a/web/src/components/product/ProductPage.jsx b/web/src/components/product/ProductPage.jsx index fb10aa993b..c85ac56d1e 100644 --- a/web/src/components/product/ProductPage.jsx +++ b/web/src/components/product/ProductPage.jsx @@ -431,7 +431,8 @@ export default function ProductPage() { const isLoading = managerStatus === BUSY || softwareStatus === BUSY; return ( - + // TRANSLATORS: page title + diff --git a/web/src/components/product/ProductPage.test.jsx b/web/src/components/product/ProductPage.test.jsx index 618d3e8994..47914dccce 100644 --- a/web/src/components/product/ProductPage.test.jsx +++ b/web/src/components/product/ProductPage.test.jsx @@ -52,11 +52,14 @@ const selectedProduct = { }; jest.mock("~/client"); - jest.mock("~/context/product", () => ({ ...jest.requireActual("~/context/product"), useProduct: () => ({ products: mockProducts, selectedProduct, registration: mockRegistration }) })); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); beforeEach(() => { mockManager = { diff --git a/web/src/components/product/ProductSelectionPage.jsx b/web/src/components/product/ProductSelectionPage.jsx index 585747a9db..76e2dc06de 100644 --- a/web/src/components/product/ProductSelectionPage.jsx +++ b/web/src/components/product/ProductSelectionPage.jsx @@ -21,12 +21,12 @@ import React, { useEffect, useState } from "react"; import { useNavigate } from "react-router-dom"; -import { Button, Form, FormGroup } from "@patternfly/react-core"; +import { Form, FormGroup } from "@patternfly/react-core"; import { _ } from "~/i18n"; -import { Icon, Loading } from "~/components/layout"; +import { Page } from "~/components/core"; +import { Loading } from "~/components/layout"; import { ProductSelector } from "~/components/product"; -import { Title, PageIcon, MainActions } from "~/components/layout/Layout"; import { useInstallerClient } from "~/context/installer"; import { useProduct } from "~/context/product"; @@ -59,22 +59,20 @@ function ProductSelectionPage() { ); return ( - <> - {/* TRANSLATORS: page header */} - {_("Product selection")} - - - - + // TRANSLATORS: page title + - + + + + { _("Select") } + + + ); } diff --git a/web/src/components/product/ProductSelectionPage.test.jsx b/web/src/components/product/ProductSelectionPage.test.jsx index 89a943a16e..8e1d392c8a 100644 --- a/web/src/components/product/ProductSelectionPage.test.jsx +++ b/web/src/components/product/ProductSelectionPage.test.jsx @@ -37,8 +37,8 @@ const products = [ description: "MicroOS description" } ]; -jest.mock("~/client"); +jest.mock("~/client"); jest.mock("~/context/product", () => ({ ...jest.requireActual("~/context/product"), useProduct: () => { @@ -48,6 +48,10 @@ jest.mock("~/context/product", () => ({ }; } })); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); const managerMock = { startProbing: jest.fn() @@ -74,10 +78,10 @@ beforeEach(() => { describe("when the user chooses a product", () => { it("selects the product and redirects to the main page", async () => { const { user } = installerRender(); - const radio = await screen.findByRole("radio", { name: "openSUSE MicroOS" }); - await user.click(radio); - const button = await screen.findByRole("button", { name: "Select" }); - await user.click(button); + const productOption = screen.getByRole("radio", { name: "openSUSE MicroOS" }); + const selectButton = screen.getByRole("button", { name: "Select" }); + await user.click(productOption); + await user.click(selectButton); expect(softwareMock.product.select).toHaveBeenCalledWith("MicroOS"); expect(managerMock.startProbing).toHaveBeenCalled(); expect(mockNavigateFn).toHaveBeenCalledWith("/"); @@ -87,9 +91,9 @@ describe("when the user chooses a product", () => { describe("when the user chooses does not change the product", () => { it("redirects to the main page", async () => { const { user } = installerRender(); - await screen.findByText("openSUSE Tumbleweed"); - const button = await screen.findByRole("button", { name: "Select" }); - await user.click(button); + screen.getByText("openSUSE Tumbleweed"); + const selectButton = await screen.findByRole("button", { name: "Select" }); + await user.click(selectButton); expect(softwareMock.product.select).not.toHaveBeenCalled(); expect(managerMock.startProbing).not.toHaveBeenCalled(); expect(mockNavigateFn).toHaveBeenCalledWith("/"); diff --git a/web/src/components/questions/Questions.test.jsx b/web/src/components/questions/Questions.test.jsx index 2cd665a9e2..ced3aeaeb0 100644 --- a/web/src/components/questions/Questions.test.jsx +++ b/web/src/components/questions/Questions.test.jsx @@ -21,7 +21,7 @@ import React from "react"; -import { act, screen, waitFor } from "@testing-library/react"; +import { act, waitFor, within } from "@testing-library/react"; import { installerRender } from "~/test-utils"; import { createClient } from "~/client"; @@ -63,24 +63,20 @@ describe("Questions", () => { }); it("renders nothing", async () => { - installerRender(); - - const main = await screen.findByRole("main"); - await waitFor(() => expect(main).toBeEmptyDOMElement()); + const { container } = installerRender(); + await waitFor(() => expect(container).toBeEmptyDOMElement()); }); }); describe("when a new question is added", () => { it("push it into the pending queue", async () => { - installerRender(); - - const main = await screen.findByRole("main"); - await waitFor(() => expect(main).toBeEmptyDOMElement()); + const { container } = installerRender(); + await waitFor(() => expect(container).toBeEmptyDOMElement()); // Manually triggers the handler given for the onQuestionAdded signal act(() => handlers.onAdd(genericQuestion)); - await screen.findByText("A Generic question mock"); + await within(container).findByText("A Generic question mock"); }); }); @@ -90,13 +86,13 @@ describe("Questions", () => { }); it("removes it from the queue", async () => { - installerRender(); - await screen.findByText("A Generic question mock"); + const { container } = installerRender(); + await within(container).findByText("A Generic question mock"); // Manually triggers the handler given for the onQuestionRemoved signal act(() => handlers.onRemove(genericQuestion.id)); - const content = screen.queryByText("A Generic question mock"); + const content = within(container).queryByText("A Generic question mock"); expect(content).toBeNull(); }); }); @@ -107,9 +103,9 @@ describe("Questions", () => { }); it("renders a GenericQuestion component", async () => { - installerRender(); + const { container } = installerRender(); - await screen.findByText("A Generic question mock"); + await within(container).findByText("A Generic question mock"); }); }); @@ -119,9 +115,9 @@ describe("Questions", () => { }); it("renders a LuksActivationQuestion component", async () => { - installerRender(); + const { container } = installerRender(); - await screen.findByText("A LUKS activation question mock"); + await within(container).findByText("A LUKS activation question mock"); }); }); }); diff --git a/web/src/components/software/SoftwarePage.jsx b/web/src/components/software/SoftwarePage.jsx index 30cab4cccb..ae57d48d97 100644 --- a/web/src/components/software/SoftwarePage.jsx +++ b/web/src/components/software/SoftwarePage.jsx @@ -78,7 +78,7 @@ function SoftwarePage() { return ( // TRANSLATORS: page title - + ); diff --git a/web/src/components/software/SoftwarePage.test.jsx b/web/src/components/software/SoftwarePage.test.jsx index 2f49e4cab7..2c62af0f0b 100644 --- a/web/src/components/software/SoftwarePage.test.jsx +++ b/web/src/components/software/SoftwarePage.test.jsx @@ -28,18 +28,6 @@ import { createClient } from "~/client"; import SoftwarePage from "./SoftwarePage"; -const SkeletonMock = "Skeleton mock"; -jest.mock("@patternfly/react-core", () => { - const original = jest.requireActual("@patternfly/react-core"); - - return { - ...original, - Skeleton: () => { return {SkeletonMock} } - }; -}); - -const PatternSelectorMock = "PatternSelector mock"; -jest.mock("~/components/software/PatternSelector", () => () => { return PatternSelectorMock }); jest.mock("~/client"); const getStatusFn = jest.fn(); const onStatusChangeFn = jest.fn(); @@ -53,17 +41,30 @@ beforeEach(() => { }; }); }); +jest.mock("@patternfly/react-core", () => { + const original = jest.requireActual("@patternfly/react-core"); + + return { + ...original, + Skeleton: () => Skeleton Mock + }; +}); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); +jest.mock("~/components/software/PatternSelector", () => () => "PatternSelector Mock"); describe("SoftwarePage", () => { it("displays a progress when the backend in busy", async () => { getStatusFn.mockResolvedValue(BUSY); await act(async () => installerRender()); - screen.getAllByText(SkeletonMock); + screen.getAllByText("Skeleton Mock"); }); it("displays the PatternSelector when the backend in ready", async () => { getStatusFn.mockResolvedValue(IDLE); await act(async () => installerRender()); - screen.getByText(PatternSelectorMock); + screen.getByText("PatternSelector Mock"); }); }); diff --git a/web/src/components/storage/DASDPage.jsx b/web/src/components/storage/DASDPage.jsx index a6ad88fa40..9f68d437ac 100644 --- a/web/src/components/storage/DASDPage.jsx +++ b/web/src/components/storage/DASDPage.jsx @@ -20,11 +20,8 @@ */ import React, { useEffect, useReducer } from "react"; -import { useNavigate } from "react-router-dom"; -import { Button } from "@patternfly/react-core"; import { _ } from "~/i18n"; -import { MainActions } from "~/components/layout"; import { If, Page } from "~/components/core"; import { DASDFormatProgress, DASDTable } from "~/components/storage"; import { useCancellablePromise } from "~/utils"; @@ -133,7 +130,6 @@ const initialState = { export default function DASDPage() { const { storage: client } = useInstallerClient(); - const navigate = useNavigate(); const { cancellablePromise } = useCancellablePromise(); const [state, dispatch] = useReducer(reducer, initialState); @@ -183,9 +179,6 @@ export default function DASDPage() { return ( // TRANSLATORS: DASD = Direct Access Storage Device, IBM mainframe storage technology - - - diff --git a/web/src/components/storage/ISCSIPage.jsx b/web/src/components/storage/ISCSIPage.jsx index e438fcbe8d..d86d7f895d 100644 --- a/web/src/components/storage/ISCSIPage.jsx +++ b/web/src/components/storage/ISCSIPage.jsx @@ -20,28 +20,17 @@ */ import React from "react"; -import { useNavigate } from "react-router-dom"; -import { Button } from "@patternfly/react-core"; import { _ } from "~/i18n"; -import { Title as PageTitle, MainActions } from "~/components/layout"; +import { Page } from "~/components/core"; import { InitiatorSection, TargetsSection } from "~/components/storage/iscsi"; export default function ISCSIPage() { - const navigate = useNavigate(); - return ( - <> - {/* TRANSLATORS: page title for iSCSI configuration */} - {_("Storage iSCSI")} - - - - + // TRANSLATORS: page title for iSCSI configuration + - + ); } diff --git a/web/src/components/storage/ProposalPage.jsx b/web/src/components/storage/ProposalPage.jsx index b9d8488697..f304cc05d0 100644 --- a/web/src/components/storage/ProposalPage.jsx +++ b/web/src/components/storage/ProposalPage.jsx @@ -215,7 +215,7 @@ export default function ProposalPage() { return ( // TRANSLATORS: page title - + diff --git a/web/src/components/storage/ProposalPage.test.jsx b/web/src/components/storage/ProposalPage.test.jsx index 11560d24af..7a61bad6cf 100644 --- a/web/src/components/storage/ProposalPage.test.jsx +++ b/web/src/components/storage/ProposalPage.test.jsx @@ -27,8 +27,6 @@ import { IDLE } from "~/client/status"; import { ProposalPage } from "~/components/storage"; jest.mock("~/client"); -jest.mock("~/components/storage/ProposalPageOptions", () => () =>
ProposalPage Options
); - jest.mock("@patternfly/react-core", () => { const original = jest.requireActual("@patternfly/react-core"); @@ -38,6 +36,11 @@ jest.mock("@patternfly/react-core", () => { }; }); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); +jest.mock("~/components/storage/ProposalPageOptions", () => () =>
ProposalPage Options
); const storageMock = { probe: jest.fn().mockResolvedValue(0), diff --git a/web/src/components/storage/ZFCPPage.jsx b/web/src/components/storage/ZFCPPage.jsx index a0ae99c56d..4042162c2f 100644 --- a/web/src/components/storage/ZFCPPage.jsx +++ b/web/src/components/storage/ZFCPPage.jsx @@ -22,12 +22,10 @@ // cspell:ignore wwpns npiv import React, { useCallback, useEffect, useReducer, useState } from "react"; -import { useNavigate } from "react-router-dom"; import { Button, Skeleton, Toolbar, ToolbarContent, ToolbarItem } from "@patternfly/react-core"; import { Table, Thead, Tr, Th, Tbody, Td } from '@patternfly/react-table'; import { _ } from "~/i18n"; -import { MainActions } from "~/components/layout"; import { If, Page, Popup, RowActions, Section, SectionSkeleton } from "~/components/core"; import { ZFCPDiskForm } from "~/components/storage"; import { noop, useCancellablePromise } from "~/utils"; @@ -666,7 +664,6 @@ const initialState = { */ export default function ZFCPPage() { const { storage: client } = useInstallerClient(); - const navigate = useNavigate(); const { cancellablePromise } = useCancellablePromise(); const [state, dispatch] = useReducer(reducer, initialState); @@ -731,11 +728,7 @@ export default function ZFCPPage() { return ( // TRANSLATORS: page title - - - - - + { const original = jest.requireActual("@patternfly/react-core"); @@ -36,6 +35,10 @@ jest.mock("@patternfly/react-core", () => { }; }); +// Since Agama sidebar is now rendered by the core/Page component, it's needed +// to mock it when testing a Page with plainRender and/or not taking care about +// sidebar's content. +jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); const controllers = [ { id: "1", channel: "0.0.fa00", active: false, lunScan: false }, @@ -70,6 +73,13 @@ beforeEach(() => { createClient.mockImplementation(() => ({ storage: { zfcp: client } })); }); +it("renders two sections: Controllers and Disks", () => { + installerRender(); + + screen.findByRole("heading", { name: "Controllers" }); + screen.findByRole("heading", { name: "Disks" }); +}); + it("loads the zFCP devices", async () => { client.getWWPNs = jest.fn().mockResolvedValue(["0x500507630703d3b3", "0x500507630704d3b3"]); installerRender(); @@ -84,13 +94,6 @@ it("loads the zFCP devices", async () => { expect(screen.getAllByRole("grid").length).toBe(2); }); -it("renders two sections: Controllers and Disks", async () => { - installerRender(); - - await screen.findByRole("heading", { name: "Controllers" }); - await screen.findByRole("heading", { name: "Disks" }); -}); - describe("if allow-lun-scan is activated", () => { beforeEach(() => { client.getAllowLUNScan = jest.fn().mockResolvedValue(true); diff --git a/web/src/components/users/RootPasswordPopup.test.jsx b/web/src/components/users/RootPasswordPopup.test.jsx index 8475e8959a..90671eeae5 100644 --- a/web/src/components/users/RootPasswordPopup.test.jsx +++ b/web/src/components/users/RootPasswordPopup.test.jsx @@ -45,10 +45,8 @@ beforeEach(() => { describe("when it is closed", () => { it("renders nothing", async () => { - installerRender(); - - const main = await screen.findByRole("main"); - await waitFor(() => expect(main).toBeEmptyDOMElement()); + const { container } = installerRender(); + await waitFor(() => expect(container).toBeEmptyDOMElement()); }); }); diff --git a/web/src/components/users/RootSSHKeyPopup.test.jsx b/web/src/components/users/RootSSHKeyPopup.test.jsx index dd479f0659..124a01c75f 100644 --- a/web/src/components/users/RootSSHKeyPopup.test.jsx +++ b/web/src/components/users/RootSSHKeyPopup.test.jsx @@ -44,10 +44,8 @@ beforeEach(() => { describe("when it is closed", () => { it("renders nothing", async () => { - installerRender(); - - const main = await screen.findByRole("main"); - await waitFor(() => expect(main).toBeEmptyDOMElement()); + const { container } = installerRender(); + await waitFor(() => expect(container).toBeEmptyDOMElement()); }); }); diff --git a/web/src/test-utils.js b/web/src/test-utils.js index 8e7a5f143d..862b624836 100644 --- a/web/src/test-utils.js +++ b/web/src/test-utils.js @@ -33,7 +33,6 @@ import { render } from "@testing-library/react"; import { createClient } from "~/client/index"; import { InstallerClientProvider } from "~/context/installer"; import { NotificationProvider } from "~/context/notification"; -import { Layout } from "~/components/layout"; import { noop } from "./utils"; import cockpit from "./lib/cockpit"; import { InstallerL10nProvider } from "./context/installerL10n"; @@ -105,7 +104,7 @@ const installerRender = (ui, options = {}) => { const Wrapper = ({ children }) => ( - {children} + {children} ); @@ -118,16 +117,11 @@ const installerRender = (ui, options = {}) => { ); }; -// Add an option to include or not the layout. const plainRender = (ui, options = {}) => { - const { layout, ...opts } = options; - if (layout) { - opts.wrapper = Layout; - } return ( { user: userEvent.setup(), - ...render(ui, opts) + ...render(ui, options) } ); }; From 27592d2e45a77c8cbb233e9e442ae4695e267422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Thu, 14 Dec 2023 12:26:26 +0000 Subject: [PATCH 08/25] [web] Drop react-teleporter dependency Due to a switch from layout slots to a page component that places everything in the right place. It has nothing to do with react-teleporter itself, which is a useful and tiny dependency that works as expected. It's simply because Agama has evolved, and it no longer needs to influence the layout from its children. Instead, the core/Page component has become a central component, and all content within the app must be contained on a page, along with the sidebar having become a static area for Agama-related information. --- web/package-lock.json | 14 -------------- web/package.json | 1 - 2 files changed, 15 deletions(-) diff --git a/web/package-lock.json b/web/package-lock.json index a587f2ad6c..8bea4c8ef9 100644 --- a/web/package-lock.json +++ b/web/package-lock.json @@ -17,7 +17,6 @@ "react": "^18.2.0", "react-dom": "^18.2.0", "react-router-dom": "^6.3.0", - "react-teleporter": "^3.1.0", "sprintf-js": "^1.1.3", "xbytes": "^1.8.0" }, @@ -15529,19 +15528,6 @@ "react-dom": ">=16.8" } }, - "node_modules/react-teleporter": { - "version": "3.1.0", - "resolved": "https://registry.npmjs.org/react-teleporter/-/react-teleporter-3.1.0.tgz", - "integrity": "sha512-GqHwE3vNiTK/0OA1+GdE1EUW7bzS+fOkfBKI/mgbXuR3w650tDycFqZvmRceJuF3asOfglBx5TUdBWCBXB4spA==", - "funding": { - "type": "github", - "url": "https://github.com/sponsors/gregberge" - }, - "peerDependencies": { - "react": "^16.8.0 || ^17.0.0 || ^18.0.0", - "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0" - } - }, "node_modules/read-pkg": { "version": "6.0.0", "resolved": "https://registry.npmjs.org/read-pkg/-/read-pkg-6.0.0.tgz", diff --git a/web/package.json b/web/package.json index c4113c9cda..350a49c96a 100644 --- a/web/package.json +++ b/web/package.json @@ -104,7 +104,6 @@ "react": "^18.2.0", "react-dom": "^18.2.0", "react-router-dom": "^6.3.0", - "react-teleporter": "^3.1.0", "sprintf-js": "^1.1.3", "xbytes": "^1.8.0" }, From 1ea51a8956d2512dc03afdb6194695f9326cd47c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Thu, 14 Dec 2023 22:25:44 +0000 Subject: [PATCH 09/25] [web] Drop CSS used by the former layout --- web/src/assets/styles/blocks.scss | 95 +++++++++++++------------- web/src/assets/styles/composition.scss | 4 +- web/src/assets/styles/layout.scss | 64 ++--------------- web/src/components/core/Page.jsx | 2 +- web/src/components/core/Sidebar.jsx | 3 +- 5 files changed, 59 insertions(+), 109 deletions(-) diff --git a/web/src/assets/styles/blocks.scss b/web/src/assets/styles/blocks.scss index 22113f29e1..d7a2d5e2c1 100644 --- a/web/src/assets/styles/blocks.scss +++ b/web/src/assets/styles/blocks.scss @@ -81,9 +81,9 @@ section > .content { border: 0; } -.sidebar { - --color-background-primary: var(--color-primary-lighter); - --wrapper-background: var(--color-gray-light); +[data-type="agama/sidebar"] { + /** Override the header background, see styles/layout.scss */ + --agama-header-bg: var(--color-primary-lighter); position: absolute; padding: 0; @@ -92,68 +92,69 @@ section > .content { inline-size: 70%; min-inline-size: min-content; box-shadow: -10px 10px 20px 0 var(--color-primary); -} - -.sidebar header { - --focus-color: var(--color-primary-darkest); -} -.sidebar footer { - border-top: 1px solid var(--color-gray); -} + header { + --focus-color: var(--color-primary-darkest); + } -.sidebar a, .sidebar button { - font-size: 16px; - font-weight: var(--fw-bold); - text-decoration: underline; - text-underline-offset: 2px; - padding-block: 0; + footer { + border-top: 1px solid var(--color-gray); + } - &:hover { - color: var(--color-link-hover); + a, button { + font-size: 16px; + font-weight: var(--fw-bold); text-decoration: underline; + text-underline-offset: 2px; + padding-block: 0; + + &:hover { + color: var(--color-link-hover); + text-decoration: underline; + + svg { + color: var(--color-link); + } + } svg { color: var(--color-link); + vertical-align: text-bottom; + margin-block-end: -2px; } } - svg { - color: var(--color-link); - vertical-align: text-bottom; - margin-block-end: -2px; - } -} + a { + margin-inline-start: var(--pf-v5-global--spacer--md); -.sidebar a { - margin-inline-start: var(--pf-v5-global--spacer--md); + // Keep links and buttons labels aligned by adding the same margin than + // .pf-v5-c-button__icon.pf-m-start + svg { + margin-inline-end: var(--pf-v5-global--spacer--xs); + } + } - // Keep links and buttons labels aligned by adding the same margin than - // .pf-v5-c-button__icon.pf-m-start - svg { - margin-inline-end: var(--pf-v5-global--spacer--xs); + // Remove not wanted PatternFly padding left on a loading link + button.pf-m-progress { + --pf-v5-c-button--m-progress--PaddingLeft: var(--pf-v5-global--spacer--md); } -} -// Remove not wanted PatternFly padding left on a loading link -.sidebar button.pf-m-progress { - --pf-v5-c-button--m-progress--PaddingLeft: var(--pf-v5-global--spacer--md); -} + button.pf-m-progress + div { + padding-inline-start: calc(var(--pf-v5-global--spacer--md)); + } -.sidebar button.pf-m-progress + div { - padding-inline-start: calc(var(--pf-v5-global--spacer--md)); -} + &[data-state="hidden"] { + transition: all 0.04s ease-in-out; + inline-size: 0; + min-inline-size: 0; + box-shadow: none; + } -.sidebar[data-state="hidden"] { - transition: all 0.04s ease-in-out; - inline-size: 0; - min-inline-size: 0; - box-shadow: none; + &[data-state="visible"] { + transition: all 0.2s ease-in-out; + } } -.sidebar[data-state="visible"] { - transition: all 0.2s ease-in-out; -} .disclosure > button { margin-inline-start: var(--pf-v5-global--spacer--md); diff --git a/web/src/assets/styles/composition.scss b/web/src/assets/styles/composition.scss index 02e15bfb26..07c7994aa7 100644 --- a/web/src/assets/styles/composition.scss +++ b/web/src/assets/styles/composition.scss @@ -30,7 +30,7 @@ body > div[inert], body > div[aria-hidden="true"], -div[data-type="agama/page-layout"] > [inert], -div[data-type="agama/page-layout"] > [aria-hidden="true"] { +div[data-type="agama/page"] > [inert], +div[data-type="agama/page"] > [aria-hidden="true"] { filter: grayscale(1) blur(2px); } diff --git a/web/src/assets/styles/layout.scss b/web/src/assets/styles/layout.scss index 542fa12c6a..6eb0adfe22 100644 --- a/web/src/assets/styles/layout.scss +++ b/web/src/assets/styles/layout.scss @@ -1,13 +1,15 @@ @use "~/assets/styles/utilities.scss"; -[data-type="agama/page-layout"] { +[data-layout="agama/base"] { + --agama-header-bg: var(--color-primary); + @extend .shadow; display: grid; block-size: 100dvh; background: white; overflow: hidden; grid-template-columns: 1fr; - grid-template-rows: auto 1fr auto; + grid-template-rows: 60px 1fr 70px; grid-template-areas: "header" "body" @@ -24,7 +26,7 @@ display: flex; align-items: center; justify-content: space-between; - background: var(--color-primary); + background: var(--agama-header-bg); color: white; fill: white; @@ -60,6 +62,7 @@ display: flex; flex-direction: row-reverse; justify-content: space-between; + align-items: center; img { inline-size: 30%; @@ -73,61 +76,6 @@ gap: var(--spacer-small); } -.wrapper { - position: relative; - display: grid; - grid-template-rows: var(--header-block-size) 1fr var(--footer-block-size); - grid-template-areas: - "header" - "content" - "footer"; - overflow: hidden; - /** block-size fallbacks **/ - height: 100vh; - height: 100dvb; - block-size: 100vh; - /** END of block-size fallbacks **/ - block-size: 100dvb; - - background: var(--wrapper-background); - - svg { - fill: currentcolor; - vertical-align: middle; - } -} - -.wrapper > * { - padding: var(--wrapper-padding); -} - -.wrapper > header { - grid-area: header; - background: var(--color-background-primary); - color: var(--color-text-secondary); - - svg { - color: white; - block-size: var(--header-icon-size); - inline-size: var(--header-icon-size); - } -} - -.wrapper > main { - grid-area: content; - overflow-y: auto; // Sadly, only Firefox supports overflow-block at this moment (Jan 2023) - overflow-block: auto; - padding: var(--spacer-medium) var(--spacer-small); -} - -.wrapper > footer { - grid-area: footer; -} - -.wrapper > footer > img { - max-inline-size: 150px; -} - [data-variant="flip-X"] { transform: scaleX(-1); } diff --git a/web/src/components/core/Page.jsx b/web/src/components/core/Page.jsx index 304399fa3f..4d9cb624b9 100644 --- a/web/src/components/core/Page.jsx +++ b/web/src/components/core/Page.jsx @@ -149,7 +149,7 @@ const Page = ({ icon, title = "Agama", children }) => { const closeSidebar = () => setSidebarOpen(false); return ( -
+

} /> diff --git a/web/src/components/core/Sidebar.jsx b/web/src/components/core/Sidebar.jsx index 7f7144dc88..e2a016c95d 100644 --- a/web/src/components/core/Sidebar.jsx +++ b/web/src/components/core/Sidebar.jsx @@ -130,8 +130,9 @@ export default function Sidebar ({ children, isOpen, onClose = noop }) {

); }; diff --git a/web/src/components/core/Sidebar.jsx b/web/src/components/core/Sidebar.jsx index 4b49267506..bf7bcf85ef 100644 --- a/web/src/components/core/Sidebar.jsx +++ b/web/src/components/core/Sidebar.jsx @@ -21,7 +21,17 @@ import React, { useCallback, useEffect, useLayoutEffect, useRef } from "react"; import { Icon } from "~/components/layout"; -import { DevelopmentInfo } from "~/components/core"; +import { InstallerKeymapSwitcher, InstallerLocaleSwitcher } from "~/components/l10n"; +import { + About, + DevelopmentInfo, + Disclosure, + IssuesLink, + // FIXME: unify names here by renaming LogsButton -> LogButton or ShowLogButton -> ShowLogsButton + LogsButton, + ShowLogButton, + ShowTerminalButton, +} from "~/components/core"; import { noop } from "~/utils"; import { _ } from "~/i18n"; import useNodeSiblings from "~/hooks/useNodeSiblings"; @@ -117,14 +127,31 @@ export default function Sidebar ({ children, isOpen, onClose = noop }) { onClick={close} ref={closeButtonRef} className="plain-control" - aria-label={_("Hide navigation and other options")} + aria-label={_("Hide installer options")} >
- {children} +
+ + + + + + + {children} + +
+
+
+
+
+
+
+
+
diff --git a/web/src/components/core/Sidebar.test.jsx b/web/src/components/core/Sidebar.test.jsx index 12875009ee..577c50abc5 100644 --- a/web/src/components/core/Sidebar.test.jsx +++ b/web/src/components/core/Sidebar.test.jsx @@ -24,6 +24,16 @@ import { screen, within } from "@testing-library/react"; import { plainRender } from "~/test-utils"; import { If, Sidebar } from "~/components/core"; +// Mock some components +jest.mock("~/components/core/About", () => () =>
About link mock
); +jest.mock("~/components/core/DevelopmentInfo", () => () =>
DevelopmentInfo mock
); +jest.mock("~/components/core/IssuesLink", () => () =>
IssuesLink mock
); +jest.mock("~/components/core/LogsButton", () => () =>
LogsButton mock
); +jest.mock("~/components/core/ShowLogButton", () => () =>
ShowLogButton mock
); +jest.mock("~/components/core/ShowTerminalButton", () => () =>
ShowTerminalButton mock
); +jest.mock("~/components/l10n/InstallerKeymapSwitcher", () => () =>
Installer keymap switcher mock
); +jest.mock("~/components/l10n/InstallerLocaleSwitcher", () => () =>
Installer locale switcher mock
); + it("renders the sidebar hidden if isOpen prop is not given", () => { plainRender(); @@ -38,6 +48,22 @@ it("renders the sidebar hidden if isOpen prop is false", () => { expect(nav).toHaveAttribute("data-state", "hidden"); }); +it("renders expected options", () => { + plainRender(); + screen.getByText("Installer keymap switcher mock"); + screen.getByText("Installer locale switcher mock"); + screen.getByText("LogsButton mock"); + screen.getByText("ShowLogButton mock"); + screen.getByText("ShowTerminalButton mock"); + screen.getByText("About link mock"); + screen.getByText("DevelopmentInfo mock"); +}); + +it("renders given children", () => { + plainRender(); + screen.getByRole("button", { name: "An extra button" }); +}); + describe("when isOpen prop is given", () => { it("renders the sidebar visible", () => { plainRender(); From 580dc4e0eba44e0a736a453f5b984f4c1127fe0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 18 Dec 2023 13:53:14 +0000 Subject: [PATCH 13/25] [web] Remove not needed mock In fact, apart from not needed it was actually malformed. --- web/src/App.test.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/web/src/App.test.jsx b/web/src/App.test.jsx index 9769a2c293..e22a279aad 100644 --- a/web/src/App.test.jsx +++ b/web/src/App.test.jsx @@ -44,7 +44,6 @@ jest.mock("~/context/product", () => ({ // Mock some components, // See https://www.chakshunyu.com/blog/how-to-mock-a-react-component-in-jest/#default-export -jest.mock("~/components/core/DBusError", () =>
D-BusError Mock
); jest.mock("~/components/questions/Questions", () => () =>
Questions Mock
); jest.mock("~/components/core/Installation", () => () =>
Installation Mock
); jest.mock("~/components/layout/Loading", () => () =>
Loading Mock
); From 5ba91b4a8ecb6d239d7e32ae6889fa4d386f64f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 18 Dec 2023 19:11:37 +0000 Subject: [PATCH 14/25] [web] Reduce needed CSS for styling core/Section Thanks to the use of CSS Subgrid, which helps to layout it as it was but using less HTML nodes and CSS rules. --- web/src/assets/styles/blocks.scss | 70 +++++++++++++---------------- web/src/assets/styles/global.scss | 1 - web/src/components/core/Section.jsx | 8 ++-- 3 files changed, 33 insertions(+), 46 deletions(-) diff --git a/web/src/assets/styles/blocks.scss b/web/src/assets/styles/blocks.scss index d7a2d5e2c1..17e97db49f 100644 --- a/web/src/assets/styles/blocks.scss +++ b/web/src/assets/styles/blocks.scss @@ -1,54 +1,44 @@ -// Standard section -// In the future we might need to use an specific CSS class for it if we start having different -// section layouts. -section:not([class^="pf-c"]) { +// CSS rules used for the standard Agama section (core/Section.jsx) +// In the future we might add different section layouts by using data-variant attribute +// or similar strategy +[data-type="agama/section"] { display: grid; - grid-template-columns: var(--section-icon-size) 1fr; - grid-template-areas: - "icon title" - ".... content"; + grid-template-rows: + [header] auto + [content] auto + ; + grid-template-columns: [bleed] var(--section-icon-size) [content] 1fr; gap: var(--spacer-small); - padding-inline-start: calc( + margin-inline-start: calc( var(--header-icon-size) - var(--section-icon-size) ); - padding-inline-end: var(--section-icon-size); -} + margin-inline-end: var(--section-icon-size); -section:not(:last-child, [class^="pf-c"]) { - margin-block-end: var(--spacer-large); -} - -section > svg { - grid-area: icon; - block-size: var(--section-icon-size); - inline-size: var(--section-icon-size); -} - -section > h2 { - grid-area: title; - - button { - border: none; - background: none; - color: inherit; - font: inherit; - padding: 0; + &:not(:last-child) { + margin-block-end: var(--spacer-normal); } - a, button { - text-decoration: underline; - text-decoration-thickness: 0.1em; - text-underline-offset: 0.2em; - transition: all 0.15s ease-in-out; + > h2 { + display: grid; + grid-area: header; + grid-template-columns: subgrid; + grid-column: bleed / content-end; - &:hover { - color: var(--color-link-hover); + svg { + block-size: var(--section-icon-size); + inline-size: var(--section-icon-size); + grid-column: bleed / content; + } + + :not(svg) { + grid-column: content } } -} -section > .content { - grid-area: content; + > :not(h2) { + grid-area: content; + grid-column: content; + } } // Custom selection list diff --git a/web/src/assets/styles/global.scss b/web/src/assets/styles/global.scss index 7576646040..8a45c36115 100644 --- a/web/src/assets/styles/global.scss +++ b/web/src/assets/styles/global.scss @@ -32,7 +32,6 @@ button.pf-m-link { &:hover { color: var(--color-link-hover); - text-decoration: underline; } } diff --git a/web/src/components/core/Section.jsx b/web/src/components/core/Section.jsx index d5d94f5fee..5f65d6479f 100644 --- a/web/src/components/core/Section.jsx +++ b/web/src/components/core/Section.jsx @@ -82,10 +82,7 @@ export default function Section({ const headerText = !path?.trim() ? title : {title}; return ( - <> - {headerIcon} -

{headerText}

- +

{headerIcon}{headerText}

); }; @@ -95,9 +92,10 @@ export default function Section({ aria-busy={loading} aria-label={ariaLabel || undefined} aria-labelledby={ title && !ariaLabel ? headerId : undefined} + data-type="agama/section" >
-
+
{errors?.length > 0 && } {children} From 546ce7354500c28f58016d8d55008714ea43b206 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 18 Dec 2023 19:52:17 +0000 Subject: [PATCH 15/25] [web] Extend core/If documentation --- web/src/components/core/If.jsx | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/web/src/components/core/If.jsx b/web/src/components/core/If.jsx index 2be4984ae4..1b373b3ec1 100644 --- a/web/src/components/core/If.jsx +++ b/web/src/components/core/If.jsx @@ -20,12 +20,43 @@ */ /** - * Simple component for simplifying conditional rendering. + * Helper component for simplifying conditional interpolation in JSX blocks. * @component * * Borrowed from the old Michael J. Ryan’s comment at https://github.com/facebook/jsx/issues/65#issuecomment-255484351 * See more options at https://blog.logrocket.com/react-conditional-rendering-9-methods/ * + * Please, use it only when "conditionally interpolating" content in a + * "JSX block". For rendering a content or null it's better to go for an early + * return. + * + * @example Using an early return instead + * ... + * if (loaded) return + * + * return ( + * + * Loading data + * + * + * + * + * ); + * ... + * + * @example Using `` in a JSX block + * ... + * return ( + * + * Loading data + * + * } else={ + * } else={ + * + * + * ); + * ... + * * @param {object} props * @param {boolean} props.condition * @param {JSX.Element} [props.then=null] - the content to be rendered when the condition is true From ba77d51f98584624989f4ceacca0ca3725a770be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 19 Dec 2023 09:12:45 +0000 Subject: [PATCH 16/25] [web] Minor documentation fixes --- web/src/components/core/If.jsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/web/src/components/core/If.jsx b/web/src/components/core/If.jsx index 1b373b3ec1..c53991d1f8 100644 --- a/web/src/components/core/If.jsx +++ b/web/src/components/core/If.jsx @@ -50,8 +50,9 @@ * * Loading data * - * } else={ - * } else={ + * } /> + * } else={} /> + * } else={} /> * * * ); From 68c8e280025f262db06c9b77f25ef74c30cfd2d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 19 Dec 2023 11:37:56 +0000 Subject: [PATCH 17/25] [web] Change how Icon sizes are handled In order to simplify the process of handling icons and improve DX, the way in which Icon components set icons' size has been changed. To begin with, the default icon size in the .svgrrc file has been changed from 48px to 28px. In that way, we don't have to create a CSS class just to override it for icons rendered with the "default" size. When theming comes into play, we'll need to reexamine this strategy as changing the default size might be challenging. In addition, the "scheme" for defining CSS rules for sizing icons has been changed from numbers to letters. I.e., .icon-xs instead of .icon-size-16. Lastly, the option to give an arbitrarily large size was added. That means that mounting an or is gonna set the width and height of the final SVG to given value without needing to define a CSS class for such size. Nonetheless, we need to revisit it later when allowing themes based on CSS Logical Properties. --- web/.svgrrc | 1 + web/__mocks__/svg.js | 4 +- web/cspell.json | 6 +- web/src/assets/styles/utilities.scss | 51 ++++--------- web/src/assets/styles/variables.scss | 15 +++- web/src/components/core/About.jsx | 2 +- web/src/components/core/Disclosure.jsx | 2 +- web/src/components/core/IssuesLink.jsx | 2 +- web/src/components/core/LogsButton.jsx | 2 +- web/src/components/core/PasswordInput.jsx | 2 +- web/src/components/core/ShowLogButton.jsx | 2 +- .../components/core/ShowTerminalButton.jsx | 2 +- web/src/components/core/Sidebar.jsx | 2 +- web/src/components/core/Tip.jsx | 2 +- web/src/components/core/ValidationErrors.jsx | 2 +- .../l10n/InstallerLocaleSwitcher.jsx | 4 +- web/src/components/layout/Icon.jsx | 26 ++++--- web/src/components/layout/Icon.test.jsx | 76 ++++++++++++------- web/src/components/layout/Loading.jsx | 2 +- .../components/network/ConnectionsTable.jsx | 2 +- web/src/components/network/NetworkPage.jsx | 2 +- .../components/network/WifiNetworkMenu.jsx | 2 +- .../components/overview/SoftwareSection.jsx | 2 +- .../questions/LuksActivationQuestion.jsx | 2 +- web/src/components/storage/DASDTable.jsx | 4 +- web/src/components/storage/ProposalPage.jsx | 2 +- .../storage/ProposalSettingsSection.jsx | 4 +- .../components/storage/ProposalVolumes.jsx | 6 +- web/src/components/storage/VolumeForm.jsx | 2 +- 29 files changed, 129 insertions(+), 104 deletions(-) diff --git a/web/.svgrrc b/web/.svgrrc index 7b2aae7d22..e181de8c14 100644 --- a/web/.svgrrc +++ b/web/.svgrrc @@ -1,3 +1,4 @@ { + "icon": 28, "plugins": ["@svgr/plugin-jsx"] } diff --git a/web/__mocks__/svg.js b/web/__mocks__/svg.js index 81ec2adbb0..f6f5bae744 100644 --- a/web/__mocks__/svg.js +++ b/web/__mocks__/svg.js @@ -2,7 +2,7 @@ import React from 'react'; export default ({...props}) => ( // Simple SVG square based on a wikimedia example https://commons.wikimedia.org/wiki/SVG_examples - - + + ); diff --git a/web/cspell.json b/web/cspell.json index 428bf4ef41..adf47ce25f 100644 --- a/web/cspell.json +++ b/web/cspell.json @@ -43,8 +43,8 @@ "ipaddr", "iscsi", "jdoe", - "keymap", - "keymaps", + "keymap", + "keymaps", "libyui", "lldp", "localdomain", @@ -68,6 +68,7 @@ "subvol", "subvolume", "subvolumes", + "svgrrc", "teleporter", "testfile", "testsuite", @@ -75,6 +76,7 @@ "tkip", "udev", "wwpn", + "xxxs", "zfcp" ] } diff --git a/web/src/assets/styles/utilities.scss b/web/src/assets/styles/utilities.scss index 39f71a946f..4adf2c18c6 100644 --- a/web/src/assets/styles/utilities.scss +++ b/web/src/assets/styles/utilities.scss @@ -34,49 +34,30 @@ font-weight: bold; } -.icon-size-10 { - width: 10px; - height: 10px; +// Utility classes for sizing icons +.icon-xxxs { + block-size: var(--icon-size-xxxs); + inline-size: var(--icon-size-xxxs); } -.icon-size-12 { - width: 12px; - height: 12px; +.icon-xxs { + block-size: var(--icon-size-xxs); + inline-size: var(--icon-size-xxs); } -.icon-size-14 { - width: 14px; - height: 14px; +.icon-xs { + block-size: var(--icon-size-xs); + inline-size: var(--icon-size-xs); } -.icon-size-15 { - width: 15px; - height: 15px; +.icon-s { + block-size: var(--icon-size-s); + inline-size: var(--icon-size-s); } -.icon-size-16 { - width: 16px; - height: 16px; -} - -.icon-size-24 { - width: 24px; - height: 24px; -} - -.icon-size-28 { - width: 28px; - height: 28px; -} - -.icon-size-32 { - width: 32px; - height: 32px; -} - -.icon-big { - inline-size: 10rem; - block-size: 10rem; +.icon-xxxl { + block-size: var(--icon-size-xxxl); + inline-size: var(--icon-size-xxxl); } .color-warn { diff --git a/web/src/assets/styles/variables.scss b/web/src/assets/styles/variables.scss index 0b01334550..ff560a7a68 100644 --- a/web/src/assets/styles/variables.scss +++ b/web/src/assets/styles/variables.scss @@ -27,6 +27,17 @@ --spacer-medium: 1.5rem; --spacer-large: 2rem; + --icon-size-xxxs: 12px; + --icon-size-xxs: 16px; + --icon-size-xs: 20px; + --icon-size-s: 24px; + --icon-size: 28px; + --icon-size-m: 32px; + --icon-size-l: 36px; + --icon-size-xl: 40px; + --icon-size-xxl: 44px; + --icon-xxxl: 10rem; + --stack-gutter: var(--spacer-normal); --split-gutter: var(--spacer-small); @@ -65,8 +76,8 @@ --gradient-border-start-color: var(--color-gray); --gradient-border-end-color: transparent; - --header-icon-size: 32px; - --section-icon-size: 28px; + --header-icon-size: var(--icon-size-m); + --section-icon-size: var(--icon-size); --header-block-size: auto; --footer-block-size: auto; diff --git a/web/src/components/core/About.jsx b/web/src/components/core/About.jsx index 6e1f68f8eb..6fcd86cc5f 100644 --- a/web/src/components/core/About.jsx +++ b/web/src/components/core/About.jsx @@ -37,7 +37,7 @@ export default function About() { <> } + icon={} onClick={open} > {_("About Agama")} diff --git a/web/src/components/core/Disclosure.jsx b/web/src/components/core/Disclosure.jsx index c1ee7542fe..c75f8c76d1 100644 --- a/web/src/components/core/Disclosure.jsx +++ b/web/src/components/core/Disclosure.jsx @@ -62,7 +62,7 @@ export default function Disclosure({ label, children, ...otherProps }) { onClick={toggle} {...otherProps} > - + {label}
diff --git a/web/src/components/core/IssuesLink.jsx b/web/src/components/core/IssuesLink.jsx index d27ae1c216..ca9c851267 100644 --- a/web/src/components/core/IssuesLink.jsx +++ b/web/src/components/core/IssuesLink.jsx @@ -36,7 +36,7 @@ export default function IssuesLink() { return ( - + {_("Show issues")} { onClick={collectAndDownload} isLoading={isCollecting} isDisabled={isCollecting} - icon={isCollecting ? null : } + icon={isCollecting ? null : } {...props} > {isCollecting ? _("Collecting logs...") : _("Download logs")} diff --git a/web/src/components/core/PasswordInput.jsx b/web/src/components/core/PasswordInput.jsx index 14a55ab58d..3e641e4f98 100644 --- a/web/src/components/core/PasswordInput.jsx +++ b/web/src/components/core/PasswordInput.jsx @@ -59,7 +59,7 @@ export default function PasswordInput({ id, ...props }) { aria-label={_("Password visibility button")} variant="control" onClick={() => setShowPassword((prev) => !prev)} - icon={} + icon={} isDisabled={props.isDisabled} /> diff --git a/web/src/components/core/ShowLogButton.jsx b/web/src/components/core/ShowLogButton.jsx index 0d2066bfb7..90aa9b10f4 100644 --- a/web/src/components/core/ShowLogButton.jsx +++ b/web/src/components/core/ShowLogButton.jsx @@ -41,7 +41,7 @@ const ShowLogButton = () => { variant="link" onClick={onClick} isDisabled={isLogDisplayed} - icon={} + icon={} > {/* TRANSLATORS: button label */} {_("Show Logs")} diff --git a/web/src/components/core/ShowTerminalButton.jsx b/web/src/components/core/ShowTerminalButton.jsx index e6cea495ba..8747605557 100644 --- a/web/src/components/core/ShowTerminalButton.jsx +++ b/web/src/components/core/ShowTerminalButton.jsx @@ -42,7 +42,7 @@ const ShowTerminalButton = () => { variant="link" onClick={onClick} isDisabled={isTermDisplayed} - icon={} + icon={} > {/* TRANSLATORS: button label */} {_("Open Terminal")} diff --git a/web/src/components/core/Sidebar.jsx b/web/src/components/core/Sidebar.jsx index bf7bcf85ef..b29020c971 100644 --- a/web/src/components/core/Sidebar.jsx +++ b/web/src/components/core/Sidebar.jsx @@ -158,7 +158,7 @@ export default function Sidebar ({ children, isOpen, onClose = noop }) { {/* TRANSLATORS: button label */} {_("Close")} - +
diff --git a/web/src/components/core/Tip.jsx b/web/src/components/core/Tip.jsx index e71f8bd74b..3185466416 100644 --- a/web/src/components/core/Tip.jsx +++ b/web/src/components/core/Tip.jsx @@ -38,7 +38,7 @@ export default function Tip ({ description, children }) { if (description) { return ( - + ); } diff --git a/web/src/components/core/ValidationErrors.jsx b/web/src/components/core/ValidationErrors.jsx index 74a19e06c6..b322afa46b 100644 --- a/web/src/components/core/ValidationErrors.jsx +++ b/web/src/components/core/ValidationErrors.jsx @@ -64,7 +64,7 @@ const ValidationErrors = ({ title = _("Errors"), errors }) => { if (!errors || errors.length === 0) return null; - const warningIcon = ; + const warningIcon = ; if (errors.length === 1) { return ( diff --git a/web/src/components/l10n/InstallerLocaleSwitcher.jsx b/web/src/components/l10n/InstallerLocaleSwitcher.jsx index 2683aa6e18..ad81c2ad7d 100644 --- a/web/src/components/l10n/InstallerLocaleSwitcher.jsx +++ b/web/src/components/l10n/InstallerLocaleSwitcher.jsx @@ -62,11 +62,11 @@ for the installed system can be set in the %s page.").split("%s"); return ( <>

- + {_("Language")}  {/* smaller width of the popover so it does not overflow outside the sidebar */} - +

- + - - + + + <>{_("Connect to a Wi-Fi network")} - - - + + + ); } diff --git a/web/src/components/network/index.js b/web/src/components/network/index.js index 7850d43466..b64ddbcd71 100644 --- a/web/src/components/network/index.js +++ b/web/src/components/network/index.js @@ -20,7 +20,7 @@ */ export { default as NetworkPage } from "./NetworkPage"; -export { default as NetworkPageOptions } from "./NetworkPageOptions"; +export { default as NetworkPageMenu } from "./NetworkPageMenu"; export { default as AddressesDataList } from "./AddressesDataList"; export { default as ConnectionsTable } from "./ConnectionsTable"; export { default as DnsDataList } from "./DnsDataList"; diff --git a/web/src/components/storage/ProposalPage.jsx b/web/src/components/storage/ProposalPage.jsx index 9f024d51e8..335c939167 100644 --- a/web/src/components/storage/ProposalPage.jsx +++ b/web/src/components/storage/ProposalPage.jsx @@ -27,7 +27,7 @@ import { useInstallerClient } from "~/context/installer"; import { toValidationError, useCancellablePromise } from "~/utils"; import { Icon } from "~/components/layout"; import { Page } from "~/components/core"; -import { ProposalActionsSection, ProposalPageOptions, ProposalSettingsSection } from "~/components/storage"; +import { ProposalActionsSection, ProposalPageMenu, ProposalSettingsSection } from "~/components/storage"; import { IDLE } from "~/client/status"; const initialState = { @@ -217,7 +217,7 @@ export default function ProposalPage() { // TRANSLATORS: page title - + ); } diff --git a/web/src/components/storage/ProposalPage.test.jsx b/web/src/components/storage/ProposalPage.test.jsx index c8b48912d8..928df81142 100644 --- a/web/src/components/storage/ProposalPage.test.jsx +++ b/web/src/components/storage/ProposalPage.test.jsx @@ -37,7 +37,7 @@ jest.mock("@patternfly/react-core", () => { }; }); jest.mock("~/components/core/Sidebar", () => () =>
Agama sidebar
); -jest.mock("~/components/storage/ProposalPageOptions", () => () =>
ProposalPage Options
); +jest.mock("~/components/storage/ProposalPageMenu", () => () =>
ProposalPage Options
); const storageMock = { probe: jest.fn().mockResolvedValue(0), diff --git a/web/src/components/storage/ProposalPageOptions.jsx b/web/src/components/storage/ProposalPageMenu.jsx similarity index 87% rename from web/src/components/storage/ProposalPageOptions.jsx rename to web/src/components/storage/ProposalPageMenu.jsx index c6df12abe3..04f97a36e5 100644 --- a/web/src/components/storage/ProposalPageOptions.jsx +++ b/web/src/components/storage/ProposalPageMenu.jsx @@ -24,7 +24,7 @@ import { useHref } from "react-router-dom"; import { _ } from "~/i18n"; import { useInstallerClient } from "~/context/installer"; -import { If, PageOptions } from "~/components/core"; +import { If, Page } from "~/components/core"; /** * Internal component for building the link to Storage/DASD page @@ -34,13 +34,13 @@ const DASDLink = () => { const href = useHref("/storage/dasd"); return ( - DASD - + ); }; @@ -52,13 +52,13 @@ const ZFCPLink = () => { const href = useHref("/storage/zfcp"); return ( - {_("zFCP")} - + ); }; @@ -70,13 +70,13 @@ const ISCSILink = () => { const href = useHref("/storage/iscsi"); return ( - {_("iSCSI")} - + ); }; @@ -84,7 +84,7 @@ const ISCSILink = () => { * Component for rendering the options available from Storage/ProposalPage * @component */ -export default function ProposalPageOptions () { +export default function ProposalPageMenu () { const [showDasdLink, setShowDasdLink] = useState(false); const [showZFCPLink, setShowZFCPLink] = useState(false); const { storage: client } = useInstallerClient(); @@ -95,12 +95,12 @@ export default function ProposalPageOptions () { }, [client.dasd, client.zfcp]); return ( - - + + } /> } /> - - + + ); } diff --git a/web/src/components/storage/ProposalPageOptions.test.jsx b/web/src/components/storage/ProposalPageMenu.test.jsx similarity index 88% rename from web/src/components/storage/ProposalPageOptions.test.jsx rename to web/src/components/storage/ProposalPageMenu.test.jsx index 928f5c7a2d..c5bde266cd 100644 --- a/web/src/components/storage/ProposalPageOptions.test.jsx +++ b/web/src/components/storage/ProposalPageMenu.test.jsx @@ -23,7 +23,7 @@ import React from "react"; import { screen } from "@testing-library/react"; import { installerRender } from "~/test-utils"; import { createClient } from "~/client"; -import { ProposalPageOptions } from "~/components/storage"; +import { ProposalPageMenu } from "~/components/storage"; jest.mock("~/client"); @@ -51,7 +51,7 @@ beforeEach(() => { }); it("contains an entry for configuring iSCSI", async () => { - const { user } = installerRender(); + const { user } = installerRender(); const toggler = screen.getByRole("button"); await user.click(toggler); const link = screen.getByRole("menuitem", { name: /iSCSI/ }); @@ -60,7 +60,7 @@ it("contains an entry for configuring iSCSI", async () => { it("contains an entry for configuring DASD when is supported", async () => { isDASDSupportedFn.mockResolvedValue(true); - const { user } = installerRender(); + const { user } = installerRender(); const toggler = screen.getByRole("button"); await user.click(toggler); const link = screen.getByRole("menuitem", { name: /DASD/ }); @@ -69,7 +69,7 @@ it("contains an entry for configuring DASD when is supported", async () => { it("does not contain an entry for configuring DASD when is NOT supported", async () => { isDASDSupportedFn.mockResolvedValue(false); - const { user } = installerRender(); + const { user } = installerRender(); const toggler = screen.getByRole("button"); await user.click(toggler); expect(screen.queryByRole("menuitem", { name: /DASD/ })).toBeNull(); @@ -77,7 +77,7 @@ it("does not contain an entry for configuring DASD when is NOT supported", async it("contains an entry for configuring zFCP when is supported", async () => { isZFCPSupportedFn.mockResolvedValue(true); - const { user } = installerRender(); + const { user } = installerRender(); const toggler = screen.getByRole("button"); await user.click(toggler); const link = screen.getByRole("menuitem", { name: /zFCP/ }); @@ -86,7 +86,7 @@ it("contains an entry for configuring zFCP when is supported", async () => { it("does not contain an entry for configuring zFCP when is NOT supported", async () => { isZFCPSupportedFn.mockResolvedValue(false); - const { user } = installerRender(); + const { user } = installerRender(); const toggler = screen.getByRole("button"); await user.click(toggler); expect(screen.queryByRole("menuitem", { name: /DASD/ })).toBeNull(); diff --git a/web/src/components/storage/index.js b/web/src/components/storage/index.js index 5dd66c988f..e43354e10e 100644 --- a/web/src/components/storage/index.js +++ b/web/src/components/storage/index.js @@ -20,7 +20,7 @@ */ export { default as ProposalPage } from "./ProposalPage"; -export { default as ProposalPageOptions } from "./ProposalPageOptions"; +export { default as ProposalPageMenu } from "./ProposalPageMenu"; export { default as ProposalSettingsSection } from "./ProposalSettingsSection"; export { default as ProposalActionsSection } from "./ProposalActionsSection"; export { default as ProposalVolumes } from "./ProposalVolumes"; From 5fb2e1f9f372a9d0d45c5e2128e3008bbd6ae5fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 19 Dec 2023 23:40:02 +0000 Subject: [PATCH 20/25] [web] Minor documentation improvements --- web/src/components/core/Page.jsx | 6 ++---- web/src/components/core/Sidebar.jsx | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/web/src/components/core/Page.jsx b/web/src/components/core/Page.jsx index 1022f48eaa..4c7d2808c7 100644 --- a/web/src/components/core/Page.jsx +++ b/web/src/components/core/Page.jsx @@ -98,10 +98,8 @@ const BackAction = () => { * Displays an installation page * @component * - * FIXME: Improve the note below - * @note Sidebar must be mounted as sibling of the page content to make it work - * as expected (allow it to be hidden, making inert siblings when open, etc). - * Remember that Sidebar is going to content only things related to Agama itself + * @note Sidebar is mounted as sibling of the page content to make it work + * as expected (e.g., changing the inert attribute of its siblings according to its visibility). * * @example Simple usage * diff --git a/web/src/components/core/Sidebar.jsx b/web/src/components/core/Sidebar.jsx index b29020c971..1d9fd88e22 100644 --- a/web/src/components/core/Sidebar.jsx +++ b/web/src/components/core/Sidebar.jsx @@ -40,8 +40,7 @@ import useNodeSiblings from "~/hooks/useNodeSiblings"; * The Agama sidebar. * @component * - * A component intended to be the place where put things exclusively related to - * the Agama installer itself. + * A component intended for placing things exclusively related to Agama. * * @param {object} props * @param {React.ReactElement} props.children From c0870bc08c3b9abc74775eadd18f51a839a09300 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 20 Dec 2023 00:08:59 +0000 Subject: [PATCH 21/25] [web] Fix icon size CSS variable --- web/src/assets/styles/variables.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/assets/styles/variables.scss b/web/src/assets/styles/variables.scss index ff560a7a68..12fb205916 100644 --- a/web/src/assets/styles/variables.scss +++ b/web/src/assets/styles/variables.scss @@ -36,7 +36,7 @@ --icon-size-l: 36px; --icon-size-xl: 40px; --icon-size-xxl: 44px; - --icon-xxxl: 10rem; + --icon-size-xxxl: 10rem; --stack-gutter: var(--spacer-normal); --split-gutter: var(--spacer-small); From 10114f4eb67c6903cad0a83cb7a94532795370a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 20 Dec 2023 00:41:45 +0000 Subject: [PATCH 22/25] [web] CSS adjustments --- web/src/assets/styles/global.scss | 4 ++++ web/src/assets/styles/layout.scss | 1 + web/src/assets/styles/patternfly-overrides.scss | 11 +++++++++++ web/src/assets/styles/utilities.scss | 6 ++++++ web/src/components/core/Tip.jsx | 5 ++++- 5 files changed, 26 insertions(+), 1 deletion(-) diff --git a/web/src/assets/styles/global.scss b/web/src/assets/styles/global.scss index 8a45c36115..0478eab4d2 100644 --- a/web/src/assets/styles/global.scss +++ b/web/src/assets/styles/global.scss @@ -55,6 +55,10 @@ th { text-align: start; } +svg { + vertical-align: middle; +} + li { svg { vertical-align: middle; diff --git a/web/src/assets/styles/layout.scss b/web/src/assets/styles/layout.scss index 6eb0adfe22..a642ff4176 100644 --- a/web/src/assets/styles/layout.scss +++ b/web/src/assets/styles/layout.scss @@ -54,6 +54,7 @@ main { grid-area: body; overflow: auto; + padding-block: var(--spacer-normal); } footer { diff --git a/web/src/assets/styles/patternfly-overrides.scss b/web/src/assets/styles/patternfly-overrides.scss index cb921d816e..47afc4c496 100644 --- a/web/src/assets/styles/patternfly-overrides.scss +++ b/web/src/assets/styles/patternfly-overrides.scss @@ -231,3 +231,14 @@ table td > .pf-v5-c-empty-state { padding: 0; vertical-align: top; } + +.pf-m-grid-md.pf-v5-c-table .pf-v5-c-menu-toggle { + padding-inline: 0; +} + +@media screen and (width <= 768px) { + .pf-m-grid-md.pf-v5-c-table tr:where(.pf-v5-c-table__tr):not(.pf-v5-c-table__expandable-row) { + padding-inline: 0; + } + +} diff --git a/web/src/assets/styles/utilities.scss b/web/src/assets/styles/utilities.scss index 4adf2c18c6..fa8621e33a 100644 --- a/web/src/assets/styles/utilities.scss +++ b/web/src/assets/styles/utilities.scss @@ -155,3 +155,9 @@ .height-75 { height: 75dvh; } + +// FIXME: drop as soon as Tip component gets rethinked / refactored +.label-tip .pf-v5-c-label__text { + display: flex; + gap: var(--spacer-smaller); +} diff --git a/web/src/components/core/Tip.jsx b/web/src/components/core/Tip.jsx index 3185466416..de68d9257a 100644 --- a/web/src/components/core/Tip.jsx +++ b/web/src/components/core/Tip.jsx @@ -38,7 +38,10 @@ export default function Tip ({ description, children }) { if (description) { return ( - + ); } From 17e7970ad9830be68386ee206b2b4808713f50bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 20 Dec 2023 00:45:38 +0000 Subject: [PATCH 23/25] [web] Reword comment --- web/src/assets/styles/utilities.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/assets/styles/utilities.scss b/web/src/assets/styles/utilities.scss index fa8621e33a..7e4a4e7aff 100644 --- a/web/src/assets/styles/utilities.scss +++ b/web/src/assets/styles/utilities.scss @@ -156,7 +156,7 @@ height: 75dvh; } -// FIXME: drop as soon as Tip component gets rethinked / refactored +// FIXME: drop as soon as Tip component gets rethought / refactored .label-tip .pf-v5-c-label__text { display: flex; gap: var(--spacer-smaller); From 45208276acb99b1222e86c9d168c659407dbbd92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 20 Dec 2023 16:26:21 +0000 Subject: [PATCH 24/25] [web] Add entry in the changes file --- web/package/cockpit-agama.changes | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/web/package/cockpit-agama.changes b/web/package/cockpit-agama.changes index a2eb4aecf8..8f7d1514e6 100644 --- a/web/package/cockpit-agama.changes +++ b/web/package/cockpit-agama.changes @@ -1,3 +1,10 @@ +------------------------------------------------------------------- +Wed Dec 20 16:25:57 UTC 2023 - David Diaz + +- Rework UI internals for improving core/Page component and changing + how layout is handled (gh#openSUSE/agama#925). +- Drop layout/Layout and stop using react-teleporter. + ------------------------------------------------------------------- Fri Dec 15 15:03:40 UTC 2023 - José Iván López González From 3511b180d980764e961cbd3bb3653499074f7f25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Thu, 21 Dec 2023 13:35:32 +0000 Subject: [PATCH 25/25] Minor grammar fix --- web/src/components/core/Page.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/components/core/Page.jsx b/web/src/components/core/Page.jsx index 4c7d2808c7..72123d843c 100644 --- a/web/src/components/core/Page.jsx +++ b/web/src/components/core/Page.jsx @@ -171,7 +171,7 @@ const Page = ({ icon, title = "Agama", children }) => { * moment is to look for children whose type ends in "PageMenu". * * @note: hot reloading could make weird things when working with this - * component because the type check. + * component because of the type check. * * @see https://stackoverflow.com/questions/55729582/check-type-of-react-component */