fix(web): restore look&feel of HTML a tags pretending to look as bu…

…ttons (#1536) #1494 introduced a typo when migrating former and temporary _src/components/core/ButtonLink_ to TypeScript, making it to stop looking as a button. Although the fix was ridicously simple, just adding the missing `e` to `scondary`, this PR takes the opportunity for improving the whole component, getting ride of a pending FIXME, using a better name for it, and also adding simple but useful unit tests.
agama-project · Aug 13, 2024 · 0f1a1d5 · 0f1a1d5
1 parent 0759ddc
commit 0f1a1d5
Show file tree

Hide file tree

Showing 12 changed files with 164 additions and 57 deletions.
diff --git 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 <[email protected]>
+
+- Allow links look like buttons again (gh#openSUSE/agama#1536).
+
 -------------------------------------------------------------------
 Fri Jul 26 11:42:05 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>
 

diff --git a/web/src/components/core/Link.test.tsx b/web/src/components/core/Link.test.tsx
@@ -0,0 +1,101 @@
+/*
+ * 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 { 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", () => {
+    installerRender(<Link to="somewhere">Agama Link</Link>);
+    const link = screen.getByRole("link", { name: "Agama Link" }) as HTMLLinkElement;
+    // NOTE: Link uses ReactRouter#useHref hook which is mocked in test-utils.js
+    // 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", () => {
+    const { rerender } = installerRender(<Link to="somewhere">Agama Link</Link>);
+    const link = screen.getByRole("link", { name: "Agama Link" });
+
+    expect(link.classList.contains("pf-m-primary")).not.toBe(true);
+
+    rerender(
+      <Link to="somewhere" isPrimary>
+        Agama Link
+      </Link>,
+    );
+    expect(link.classList.contains("pf-m-primary")).toBe(true);
+
+    rerender(
+      <Link to="somewhere" isPrimary={false}>
+        {" "}
+        Agama Link
+      </Link>,
+    );
+    expect(link.classList.contains("pf-m-primary")).not.toBe(true);
+
+    rerender(
+      <Link to="somewhere" variant="primary">
+        Agama Link
+      </Link>,
+    );
+    expect(link.classList.contains("pf-m-primary")).toBe(true);
+
+    rerender(
+      <Link to="somewhere" isPrimary={false} variant="primary">
+        {" "}
+        Agama Link
+      </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 } = installerRender(<Link to="somewhere">Agama Link</Link>);
+    const link = screen.getByRole("link", { name: "Agama Link" });
+
+    expect(link.classList.contains("pf-m-secondary")).toBe(true);
+
+    rerender(
+      <Link to="somewhere" isPrimary={false}>
+        Agama Link
+      </Link>,
+    );
+    expect(link.classList.contains("pf-m-secondary")).toBe(true);
+
+    rerender(
+      // Unexpected, but possible since isPrimary is just a "helper"
+      <Link to="somewhere" isPrimary={false} variant="primary">
+        Agama Link
+      </Link>,
+    );
+    expect(link.classList.contains("pf-m-secondary")).not.toBe(true);
+
+    rerender(
+      <Link to="somewhere" variant="link">
+        Agama Link
+      </Link>,
+    );
+    expect(link.classList.contains("pf-m-secondary")).not.toBe(true);
+  });
+});
diff --git a/web/src/components/core/ButtonLink.tsx → web/src/components/core/Link.tsx b/web/src/components/core/ButtonLink.tsx → web/src/components/core/Link.tsx
@@ -20,22 +20,28 @@
  */
 
 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 { useHref } 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<ButtonProps, "component"> & {
+  /** The target route */
+  to: string;
+  /** Whether use PF/Button primary variant */
+  isPrimary?: boolean;
+};
 
-export default function ButtonLink({ to, isPrimary = false, children, ...props }) {
+/**
+ * Returns an HTML `<a>` tag built on top of PF/Button and useHref ReactRouter hook
+ *
+ * @note when isPrimary not given or false and props does not contain a variant prop,
+ * it will default to "secondary" variant
+ */
+export default function Link({ to, isPrimary, variant, children, ...props }: LinkProps) {
+  const href = useHref(to);
+  const linkVariant = isPrimary ? "primary" : variant || "secondary";
   return (
-    <Link
-      to={to}
-      className={[buttonStyles.button, buttonStyles.modifiers[isPrimary ? "primary" : "scondary"]]
-        .join(" ")
-        .trim()}
-      {...props}
-    >
+    <Button component="a" href={href} variant={linkVariant} {...props}>
       {children}
-    </Link>
+    </Button>
   );
 }
diff --git 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
@@ -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")}
             >
-              <ButtonLink to={PATHS.localeSelection} isPrimary={!locale}>
+              <Link to={PATHS.localeSelection} isPrimary={!locale}>
                 {locale ? _("Change") : _("Select")}
-              </ButtonLink>
+              </Link>
             </Section>
           </GalleryItem>
 
           <GalleryItem>
             <Section label={_("Keyboard")} value={keymap ? keymap.name : _("Not selected yet")}>
-              <ButtonLink to={PATHS.keymapSelection} isPrimary={!keymap}>
+              <Link to={PATHS.keymapSelection} isPrimary={!keymap}>
                 {keymap ? _("Change") : _("Select")}
-              </ButtonLink>
+              </Link>
             </Section>
           </GalleryItem>
 
@@ -74,9 +74,9 @@ export default function L10nPage() {
               label={_("Time zone")}
               value={timezone ? (timezone.parts || []).join(" - ") : _("Not selected yet")}
             >
-              <ButtonLink to={PATHS.timezoneSelection} isPrimary={!timezone}>
+              <Link to={PATHS.timezoneSelection} isPrimary={!timezone}>
                 {timezone ? _("Change") : _("Select")}
-              </ButtonLink>
+              </Link>
             </Section>
           </GalleryItem>
         </Gallery>

diff --git a/web/src/components/l10n/L10nPage.test.jsx b/web/src/components/l10n/L10nPage.test.jsx
@@ -20,7 +20,8 @@
  */
 
 import React from "react";
-import { render, screen, within } from "@testing-library/react";
+import { screen, within } from "@testing-library/react";
+import { installerRender } from "~/test-utils";
 import L10nPage from "~/components/l10n/L10nPage";
 
 let mockLoadedData;
@@ -40,12 +41,6 @@ const timezones = [
   { id: "Europe/Madrid", parts: ["Europe", "Madrid"] },
 ];
 
-jest.mock("react-router-dom", () => ({
-  ...jest.requireActual("react-router-dom"),
-  // TODO: mock the link because it needs a working router.
-  Link: ({ children }) => <button>{children}</button>,
-}));
-
 jest.mock("~/queries/l10n", () => ({
   useL10n: () => mockLoadedData,
 }));
@@ -62,7 +57,7 @@ beforeEach(() => {
 });
 
 it("renders a section for configuring the language", () => {
-  render(<L10nPage />);
+  installerRender(<L10nPage />);
   const region = screen.getByRole("region", { name: "Language" });
   within(region).getByText("English - United States");
   within(region).getByText("Change");
@@ -74,15 +69,15 @@ describe("if there is no selected language", () => {
   });
 
   it("renders a button for selecting a language", () => {
-    render(<L10nPage />);
+    installerRender(<L10nPage />);
     const region = screen.getByRole("region", { name: "Language" });
     within(region).getByText("Not selected yet");
     within(region).getByText("Select");
   });
 });
 
 it("renders a section for configuring the keyboard", () => {
-  render(<L10nPage />);
+  installerRender(<L10nPage />);
   const region = screen.getByRole("region", { name: "Keyboard" });
   within(region).getByText("English");
   within(region).getByText("Change");
@@ -94,15 +89,15 @@ describe("if there is no selected keyboard", () => {
   });
 
   it("renders a button for selecting a keyboard", () => {
-    render(<L10nPage />);
+    installerRender(<L10nPage />);
     const region = screen.getByRole("region", { name: "Keyboard" });
     within(region).getByText("Not selected yet");
     within(region).getByText("Select");
   });
 });
 
 it("renders a section for configuring the time zone", () => {
-  render(<L10nPage />);
+  installerRender(<L10nPage />);
   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(<L10nPage />);
+    installerRender(<L10nPage />);
     const region = screen.getByRole("region", { name: "Time zone" });
     within(region).getByText("Not selected yet");
     within(region).getByText("Select");

diff --git 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() {
       <CardField
         label={_("Wi-Fi")}
         actions={
-          <ButtonLink isPrimary={!activeConnection} to={PATHS.wifis}>
+          <Link isPrimary={!activeConnection} to={PATHS.wifis}>
             {activeConnection ? _("Change") : _("Connect")}
-          </ButtonLink>
+          </Link>
         }
       >
         <CardField.Content>

diff --git 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 (
       <Split hasGutter>
-        <ButtonLink onClick={async () => await client.network.connectTo(network.settings)}>
+        <Link onClick={async () => await client.network.connectTo(network.settings)}>
           {_("Connect")}
-        </ButtonLink>
-        <ButtonLink to={generatePath(PATHS.editConnection, { id: network.settings.id })}>
+        </Link>
+        <Link to={generatePath(PATHS.editConnection, { id: network.settings.id })}>
           {_("Edit")}
-        </ButtonLink>
+        </Link>
         <Button variant="secondary" isDanger onClick={forgetNetwork}>
           {_("Forget")}
         </Button>
@@ -132,12 +132,12 @@ const WifiDrawerPanelBody = ({ network, onCancel }) => {
         <Stack>
           <ConnectionData network={network} />
           <Split hasGutter>
-            <ButtonLink onClick={async () => await client.network.disconnect(network.settings)}>
+            <Link onClick={async () => await client.network.disconnect(network.settings)}>
               {_("Disconnect")}
-            </ButtonLink>
-            <ButtonLink to={generatePath(PATHS.editConnection, { id: network.settings.id })}>
+            </Link>
+            <Link to={generatePath(PATHS.editConnection, { id: network.settings.id })}>
               {_("Edit")}
-            </ButtonLink>
+            </Link>
             <Button
               variant="secondary"
               isDanger

diff --git a/web/src/components/software/SoftwarePage.tsx b/web/src/components/software/SoftwarePage.tsx
@@ -30,7 +30,7 @@ import {
   GridItem,
   Stack,
 } from "@patternfly/react-core";
-import { ButtonLink, CardField, IssuesHint, Page } from "~/components/core";
+import { Link, CardField, IssuesHint, Page } from "~/components/core";
 import UsedSize from "./UsedSize";
 import { useIssues } from "~/queries/issues";
 import { usePatterns, useProposal, useProposalChanges } from "~/queries/software";
@@ -67,9 +67,9 @@ const SelectedPatterns = ({ patterns }): React.ReactNode => (
   <CardField
     label={_("Selected patterns")}
     actions={
-      <ButtonLink to={PATHS.patternsSelection} isPrimary>
+      <Link to={PATHS.patternsSelection} isPrimary>
         {_("Change selection")}
-      </ButtonLink>
+      </Link>
     }
   >
     <CardBody>

diff --git a/web/src/components/storage/InstallationDeviceField.jsx b/web/src/components/storage/InstallationDeviceField.jsx
@@ -23,7 +23,7 @@
 
 import React from "react";
 import { Skeleton } from "@patternfly/react-core";
-import { ButtonLink, CardField } from "~/components/core";
+import { Link, CardField } from "~/components/core";
 import { deviceLabel } from "~/components/storage/utils";
 import { PATHS } from "~/routes/storage";
 import { _ } from "~/i18n";
@@ -105,9 +105,9 @@ export default function InstallationDeviceField({
         isLoading ? (
           <Skeleton fontSize="sm" width="100px" />
         ) : (
-          <ButtonLink to={PATHS.targetDevice} isPrimary={false}>
+          <Link to={PATHS.targetDevice} isPrimary={false}>
             {_("Change")}
-          </ButtonLink>
+          </Link>
         )
       }
     >