From 6766c5d8b5a1b33201180cd68e9c8af74226132f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 13 Aug 2024 13:37:29 +0100 Subject: [PATCH 1/8] fix(web): use the right index for getting a style modifier There were a typo in "secondary" making links does not appears as secondary PF/Buttons --- web/src/components/core/ButtonLink.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/components/core/ButtonLink.tsx b/web/src/components/core/ButtonLink.tsx index 6031085014..d20009aba5 100644 --- a/web/src/components/core/ButtonLink.tsx +++ b/web/src/components/core/ButtonLink.tsx @@ -30,7 +30,7 @@ export default function ButtonLink({ to, isPrimary = false, children, ...props } return ( Date: Tue, 13 Aug 2024 13:58:53 +0100 Subject: [PATCH 2/8] fix(web): change the core/ButtonLink approach To use a PF/Button and a ReactRouter/useNavigate hook instead of the ReactRouter/Link component. It helps to simplify the code and easily keep a visual consistence. --- web/src/components/core/ButtonLink.tsx | 29 ++++++++++++++------------ 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/web/src/components/core/ButtonLink.tsx b/web/src/components/core/ButtonLink.tsx index d20009aba5..4b407308fe 100644 --- a/web/src/components/core/ButtonLink.tsx +++ b/web/src/components/core/ButtonLink.tsx @@ -20,22 +20,25 @@ */ import React from "react"; -import { Link } from "react-router-dom"; -import buttonStyles from "@patternfly/react-styles/css/components/Button/button"; +import { Button, ButtonProps } from "@patternfly/react-core"; +import { useNavigate } from "react-router-dom"; -// TODO: Evaluate which is better, this approach or just using a -// PF/Button with onClick callback and "component" prop sets as "a" +type LinkProps = Omit & { + /** The target route */ + to: string; +}; -export default function ButtonLink({ to, isPrimary = false, children, ...props }) { +/** + * Returns an HTML `` tag built on top of PF/Button and useNavigate ReactRouter hook + * + * By default, it uses the PF/Button "secondary" variant to make the link look like a button but + * using the right HTML tag for a navigation action. + */ +export default function ButtonLink({ to, children, ...props }: LinkProps) { + const navigate = useNavigate(); return ( - + ); } From 7e2896aae5598d0c6be655a1ace545b8c0b37ca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 13 Aug 2024 15:24:01 +0100 Subject: [PATCH 3/8] refactor(web): rename ButtonLink -> Link and improves it --- web/src/components/core/Link.test.tsx | 100 ++++++++++++++++++ .../core/{ButtonLink.tsx => Link.tsx} | 17 +-- web/src/components/core/index.js | 2 +- web/src/components/l10n/L10nPage.jsx | 14 +-- web/src/components/network/NetworkPage.jsx | 6 +- .../network/WifiNetworksListPage.jsx | 18 ++-- web/src/components/software/SoftwarePage.tsx | 6 +- .../storage/InstallationDeviceField.jsx | 6 +- .../storage/ProposalActionsSummary.jsx | 4 +- web/src/components/users/FirstUser.jsx | 6 +- 10 files changed, 141 insertions(+), 38 deletions(-) create mode 100644 web/src/components/core/Link.test.tsx rename web/src/components/core/{ButtonLink.tsx => Link.tsx} (63%) diff --git a/web/src/components/core/Link.test.tsx b/web/src/components/core/Link.test.tsx new file mode 100644 index 0000000000..bed6993aad --- /dev/null +++ b/web/src/components/core/Link.test.tsx @@ -0,0 +1,100 @@ +/* + * Copyright (c) [2024] 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 { screen } from "@testing-library/react"; +import { plainRender } from "~/test-utils"; +import { Link } from "~/components/core"; + +describe("Link", () => { + it("renders an HTML `a` tag with the `href` attribute set to given `to` prop", () => { + plainRender(Agama Link); + const link = screen.getByRole("link", { name: "Agama Link" }); + // NOTE: Link uses ReactRouter#useHref hook which is mocked in test-utils.js + expect(link).toHaveAttribute("href", "somewhere"); + }); + + it("renders it as primary when either, using a truthy `isPrimary` prop or `variant` is set to primary", () => { + const { rerender } = plainRender(Agama Link); + const link = screen.getByRole("link", { name: "Agama Link" }); + + expect(link.classList.contains("pf-m-primary")).not.toBe(true); + + rerender( + + Agama Link + , + ); + expect(link.classList.contains("pf-m-primary")).toBe(true); + + rerender( + + {" "} + Agama Link + , + ); + expect(link.classList.contains("pf-m-primary")).not.toBe(true); + + rerender( + + Agama Link + , + ); + expect(link.classList.contains("pf-m-primary")).toBe(true); + + rerender( + + {" "} + Agama Link + , + ); + expect(link.classList.contains("pf-m-primary")).toBe(true); + }); + + it("renders it as secondary when neither is given, a truthy `isPrimary` nor `variant`", () => { + const { rerender } = plainRender(Agama Link); + const link = screen.getByRole("link", { name: "Agama Link" }); + + expect(link.classList.contains("pf-m-secondary")).toBe(true); + + rerender( + + Agama Link + , + ); + expect(link.classList.contains("pf-m-secondary")).toBe(true); + + rerender( + // Unexpected, but possible since isPrimary is just a "helper" + + Agama Link + , + ); + expect(link.classList.contains("pf-m-secondary")).not.toBe(true); + + rerender( + + Agama Link + , + ); + expect(link.classList.contains("pf-m-secondary")).not.toBe(true); + }); +}); diff --git a/web/src/components/core/ButtonLink.tsx b/web/src/components/core/Link.tsx similarity index 63% rename from web/src/components/core/ButtonLink.tsx rename to web/src/components/core/Link.tsx index 4b407308fe..800076a3b7 100644 --- a/web/src/components/core/ButtonLink.tsx +++ b/web/src/components/core/Link.tsx @@ -21,23 +21,26 @@ import React from "react"; import { Button, ButtonProps } from "@patternfly/react-core"; -import { useNavigate } from "react-router-dom"; +import { useHref } from "react-router-dom"; type LinkProps = Omit & { /** The target route */ to: string; + /** Whether use PF/Button primary variant */ + isPrimary?: boolean; }; /** - * Returns an HTML `` tag built on top of PF/Button and useNavigate ReactRouter hook + * Returns an HTML `` tag built on top of PF/Button and useHref ReactRouter hook * - * By default, it uses the PF/Button "secondary" variant to make the link look like a button but - * using the right HTML tag for a navigation action. + * @note when isPrimary not given or false and props does not contain a variant prop, + * it will default to "secondary" variant */ -export default function ButtonLink({ to, children, ...props }: LinkProps) { - const navigate = useNavigate(); +export default function Link({ to, isPrimary, variant, children, ...props }: LinkProps) { + const href = useHref(to); + const linkVariant = isPrimary ? "primary" : variant || "secondary"; return ( - ); diff --git a/web/src/components/core/index.js b/web/src/components/core/index.js index ada3c45abf..d755b19ce4 100644 --- a/web/src/components/core/index.js +++ b/web/src/components/core/index.js @@ -51,7 +51,7 @@ export { default as ServerError } from "./ServerError"; export { default as ExpandableSelector } from "./ExpandableSelector"; export { default as TreeTable } from "./TreeTable"; export { default as CardField } from "./CardField"; -export { default as ButtonLink } from "./ButtonLink"; +export { default as Link } from "./Link"; export { default as EmptyState } from "./EmptyState"; export { default as InstallerOptions } from "./InstallerOptions"; export { default as Drawer } from "./Drawer"; diff --git a/web/src/components/l10n/L10nPage.jsx b/web/src/components/l10n/L10nPage.jsx index 790628a901..8e01bffbfc 100644 --- a/web/src/components/l10n/L10nPage.jsx +++ b/web/src/components/l10n/L10nPage.jsx @@ -21,7 +21,7 @@ import React from "react"; import { Gallery, GalleryItem } from "@patternfly/react-core"; -import { ButtonLink, CardField, Page } from "~/components/core"; +import { Link, CardField, Page } from "~/components/core"; import { PATHS } from "~/routes/l10n"; import { _ } from "~/i18n"; import { useL10n } from "~/queries/l10n"; @@ -55,17 +55,17 @@ export default function L10nPage() { label={_("Language")} value={locale ? `${locale.name} - ${locale.territory}` : _("Not selected yet")} > - + {locale ? _("Change") : _("Select")} - +
- + {keymap ? _("Change") : _("Select")} - +
@@ -74,9 +74,9 @@ export default function L10nPage() { label={_("Time zone")} value={timezone ? (timezone.parts || []).join(" - ") : _("Not selected yet")} > - + {timezone ? _("Change") : _("Select")} - + diff --git a/web/src/components/network/NetworkPage.jsx b/web/src/components/network/NetworkPage.jsx index dd1216d346..e68edf8dac 100644 --- a/web/src/components/network/NetworkPage.jsx +++ b/web/src/components/network/NetworkPage.jsx @@ -23,7 +23,7 @@ import React from "react"; import { CardBody, Grid, GridItem } from "@patternfly/react-core"; -import { ButtonLink, CardField, EmptyState, Page } from "~/components/core"; +import { Link, CardField, EmptyState, Page } from "~/components/core"; import { ConnectionsTable } from "~/components/network"; import { formatIp } from "~/client/network/utils"; import { useNetwork, useNetworkConfigChanges } from "~/queries/network"; @@ -71,9 +71,9 @@ export default function NetworkPage() { + {activeConnection ? _("Change") : _("Connect")} - + } > diff --git a/web/src/components/network/WifiNetworksListPage.jsx b/web/src/components/network/WifiNetworksListPage.jsx index 12e362a298..255524f4a0 100644 --- a/web/src/components/network/WifiNetworksListPage.jsx +++ b/web/src/components/network/WifiNetworksListPage.jsx @@ -47,7 +47,7 @@ import { generatePath } from "react-router-dom"; import { useQueryClient } from "@tanstack/react-query"; import { Icon } from "~/components/layout"; import { WifiConnectionForm } from "~/components/network"; -import { ButtonLink } from "~/components/core"; +import { Link } from "~/components/core"; import { DeviceState } from "~/client/network/model"; import { formatIp } from "~/client/network/utils"; import { useInstallerClient } from "~/context/installer"; @@ -108,12 +108,12 @@ const WifiDrawerPanelBody = ({ network, onCancel }) => { if (network.settings && !network.device) { return ( - await client.network.connectTo(network.settings)}> + await client.network.connectTo(network.settings)}> {_("Connect")} - - + + {_("Edit")} - + @@ -132,12 +132,12 @@ const WifiDrawerPanelBody = ({ network, onCancel }) => { - await client.network.disconnect(network.settings)}> + await client.network.disconnect(network.settings)}> {_("Disconnect")} - - + + {_("Edit")} - + , -})); - jest.mock("~/queries/l10n", () => ({ useL10n: () => mockLoadedData, })); @@ -62,7 +57,7 @@ beforeEach(() => { }); it("renders a section for configuring the language", () => { - render(); + plainRender(); const region = screen.getByRole("region", { name: "Language" }); within(region).getByText("English - United States"); within(region).getByText("Change"); @@ -74,7 +69,7 @@ describe("if there is no selected language", () => { }); it("renders a button for selecting a language", () => { - render(); + plainRender(); const region = screen.getByRole("region", { name: "Language" }); within(region).getByText("Not selected yet"); within(region).getByText("Select"); @@ -82,7 +77,7 @@ describe("if there is no selected language", () => { }); it("renders a section for configuring the keyboard", () => { - render(); + plainRender(); const region = screen.getByRole("region", { name: "Keyboard" }); within(region).getByText("English"); within(region).getByText("Change"); @@ -94,7 +89,7 @@ describe("if there is no selected keyboard", () => { }); it("renders a button for selecting a keyboard", () => { - render(); + plainRender(); const region = screen.getByRole("region", { name: "Keyboard" }); within(region).getByText("Not selected yet"); within(region).getByText("Select"); @@ -102,7 +97,7 @@ describe("if there is no selected keyboard", () => { }); it("renders a section for configuring the time zone", () => { - render(); + plainRender(); const region = screen.getByRole("region", { name: "Time zone" }); within(region).getByText("Europe - Berlin"); within(region).getByText("Change"); @@ -114,7 +109,7 @@ describe("if there is no selected time zone", () => { }); it("renders a button for selecting a time zone", () => { - render(); + plainRender(); const region = screen.getByRole("region", { name: "Time zone" }); within(region).getByText("Not selected yet"); within(region).getByText("Select"); From fbc88e122d201e0693e3a14fa1772d47b4457ab8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 13 Aug 2024 15:55:19 +0100 Subject: [PATCH 5/8] fix(web): use installerRender instead of plainRender Because the MemoryRouter used for testing is only mounted when unsing installerRender. Most probably, something to review in a future iteration of testing improvements. --- web/src/components/core/Link.test.tsx | 8 ++++---- web/src/components/l10n/L10nPage.test.jsx | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/web/src/components/core/Link.test.tsx b/web/src/components/core/Link.test.tsx index bed6993aad..8bb617e0f4 100644 --- a/web/src/components/core/Link.test.tsx +++ b/web/src/components/core/Link.test.tsx @@ -21,19 +21,19 @@ import React from "react"; import { screen } from "@testing-library/react"; -import { plainRender } from "~/test-utils"; +import { installerRender } from "~/test-utils"; import { Link } from "~/components/core"; describe("Link", () => { it("renders an HTML `a` tag with the `href` attribute set to given `to` prop", () => { - plainRender(Agama Link); + installerRender(Agama Link); const link = screen.getByRole("link", { name: "Agama Link" }); // NOTE: Link uses ReactRouter#useHref hook which is mocked in test-utils.js expect(link).toHaveAttribute("href", "somewhere"); }); it("renders it as primary when either, using a truthy `isPrimary` prop or `variant` is set to primary", () => { - const { rerender } = plainRender(Agama Link); + const { rerender } = installerRender(Agama Link); const link = screen.getByRole("link", { name: "Agama Link" }); expect(link.classList.contains("pf-m-primary")).not.toBe(true); @@ -70,7 +70,7 @@ describe("Link", () => { }); it("renders it as secondary when neither is given, a truthy `isPrimary` nor `variant`", () => { - const { rerender } = plainRender(Agama Link); + const { rerender } = installerRender(Agama Link); const link = screen.getByRole("link", { name: "Agama Link" }); expect(link.classList.contains("pf-m-secondary")).toBe(true); diff --git a/web/src/components/l10n/L10nPage.test.jsx b/web/src/components/l10n/L10nPage.test.jsx index 6fc5cec858..3737fb0ebc 100644 --- a/web/src/components/l10n/L10nPage.test.jsx +++ b/web/src/components/l10n/L10nPage.test.jsx @@ -21,7 +21,7 @@ import React from "react"; import { screen, within } from "@testing-library/react"; -import { plainRender } from "~/test-utils"; +import { installerRender } from "~/test-utils"; import L10nPage from "~/components/l10n/L10nPage"; let mockLoadedData; @@ -57,7 +57,7 @@ beforeEach(() => { }); it("renders a section for configuring the language", () => { - plainRender(); + installerRender(); const region = screen.getByRole("region", { name: "Language" }); within(region).getByText("English - United States"); within(region).getByText("Change"); @@ -69,7 +69,7 @@ describe("if there is no selected language", () => { }); it("renders a button for selecting a language", () => { - plainRender(); + installerRender(); const region = screen.getByRole("region", { name: "Language" }); within(region).getByText("Not selected yet"); within(region).getByText("Select"); @@ -77,7 +77,7 @@ describe("if there is no selected language", () => { }); it("renders a section for configuring the keyboard", () => { - plainRender(); + installerRender(); const region = screen.getByRole("region", { name: "Keyboard" }); within(region).getByText("English"); within(region).getByText("Change"); @@ -89,7 +89,7 @@ describe("if there is no selected keyboard", () => { }); it("renders a button for selecting a keyboard", () => { - plainRender(); + installerRender(); const region = screen.getByRole("region", { name: "Keyboard" }); within(region).getByText("Not selected yet"); within(region).getByText("Select"); @@ -97,7 +97,7 @@ describe("if there is no selected keyboard", () => { }); it("renders a section for configuring the time zone", () => { - plainRender(); + installerRender(); const region = screen.getByRole("region", { name: "Time zone" }); within(region).getByText("Europe - Berlin"); within(region).getByText("Change"); @@ -109,7 +109,7 @@ describe("if there is no selected time zone", () => { }); it("renders a button for selecting a time zone", () => { - plainRender(); + installerRender(); const region = screen.getByRole("region", { name: "Time zone" }); within(region).getByText("Not selected yet"); within(region).getByText("Select"); From 6a01b04ff4e8a10d6a16a1bb06bef0b106002b4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 13 Aug 2024 16:05:02 +0100 Subject: [PATCH 6/8] fix(web): avoid testing errors in CI For some reason, the test worked as it was locally but failed at CI. --- web/src/components/core/Link.test.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/web/src/components/core/Link.test.tsx b/web/src/components/core/Link.test.tsx index 8bb617e0f4..46075c7d0d 100644 --- a/web/src/components/core/Link.test.tsx +++ b/web/src/components/core/Link.test.tsx @@ -27,9 +27,10 @@ import { Link } from "~/components/core"; describe("Link", () => { it("renders an HTML `a` tag with the `href` attribute set to given `to` prop", () => { installerRender(Agama Link); - const link = screen.getByRole("link", { name: "Agama Link" }); + const link = screen.getByRole("link", { name: "Agama Link" }) as HTMLLinkElement; // NOTE: Link uses ReactRouter#useHref hook which is mocked in test-utils.js - expect(link).toHaveAttribute("href", "somewhere"); + // TODO: Not using toHaveAttribute("href", "somewhere") because some weird problems at CI + expect(link.href).toContain("somewhere"); }); it("renders it as primary when either, using a truthy `isPrimary` prop or `variant` is set to primary", () => { From 23bb4b2b2eb0c27daff8127a45320ecb489dd208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 13 Aug 2024 16:08:39 +0100 Subject: [PATCH 7/8] doc(web): add entry in changes file --- web/package/agama-web-ui.changes | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/web/package/agama-web-ui.changes b/web/package/agama-web-ui.changes index e24a93e7fb..59e64d6d5b 100644 --- a/web/package/agama-web-ui.changes +++ b/web/package/agama-web-ui.changes @@ -1,3 +1,8 @@ +------------------------------------------------------------------- +Tue Aug 13 14:57:21 UTC 2024 - David Diaz + +- Allow links looks like buttons again (gh#openSUSE/agama#1536). + ------------------------------------------------------------------- Fri Jul 26 11:42:05 UTC 2024 - Imobach Gonzalez Sosa From bd476a33ab3fce16727873d371d9639137cfe91b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 13 Aug 2024 16:13:16 +0100 Subject: [PATCH 8/8] fix(web): suggestion from code review --- web/package/agama-web-ui.changes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/package/agama-web-ui.changes b/web/package/agama-web-ui.changes index 59e64d6d5b..3f0b2a424d 100644 --- a/web/package/agama-web-ui.changes +++ b/web/package/agama-web-ui.changes @@ -1,7 +1,7 @@ ------------------------------------------------------------------- Tue Aug 13 14:57:21 UTC 2024 - David Diaz -- Allow links looks like buttons again (gh#openSUSE/agama#1536). +- Allow links look like buttons again (gh#openSUSE/agama#1536). ------------------------------------------------------------------- Fri Jul 26 11:42:05 UTC 2024 - Imobach Gonzalez Sosa