Skip to content

Commit

Permalink
feat(web): move installation issues to a drawer and keep Install butt…
Browse files Browse the repository at this point in the history
…on always visible (#1778)

## Problem

[A month ago](#1690) , the
'Install' button achieved more visibility after being moved to the
sticky header, allowing users to access it from any page/section/screen.

But that was just an intermediate step towards improving the 'Install'
action and the discoverability of 'Installation setup problems,' which
sadly introduced some drawbacks:

* The Install button is displayed dynamically, which still _forces_
users to figure out where it will appear when it’s not visible from the
start.
* When installation is not possible, users have to navigate to the
Overview page to see what is making their setup invalid.


## Solution

* Keep the install button always enabled and visible, triggering the
installation when the setup is valid or displaying found _issues_ when
not, and
* Allow users to check found problems always, not only  in the overview 

## A discarded alternative

At commit
6ecdceb,
this set of changes was in a bit different shape, where the interface
rendered a disabled "Install" button and an enabled "Warning" button to
display the setup problems. Additionally, it featured a _tooltip_ to
explain to users why the "Install" button was disabled and where to
click to view the reasons in detail.

However, this approach created unnecessary complexity and moving pieces,
not to mention the challenge of writing a clear, concise, yet complete
explanation for the _tooltip_ and keeping it up-to-date, including
translations

What's more, there is no reason to avoid keeping the "Install" button
**always active but behaving slightly different** depending on the setup
status:

* Displaying found problems when the installation setup is invalid
* Displaying the confirmation dialog when the setup is valid

This way, the user doesn't need to think. They can simply click the
"Install" button. If it's not possible, they can immediately start
fixing their setup. If it is possible, they just have to confirm they
want to begin the installation.

---

Related to not yet planned Trello card, https://trello.com/c/DmUOQN1Y
(internal link)
  • Loading branch information
dgdavid authored Nov 27, 2024
2 parents ccc33bb + aef1ffe commit e721f35
Show file tree
Hide file tree
Showing 16 changed files with 378 additions and 328 deletions.
7 changes: 7 additions & 0 deletions web/package/agama-web-ui.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
-------------------------------------------------------------------
Mon Nov 25 11:12:36 UTC 2024 - David Diaz <[email protected]>

- Unify Install and "warning" header buttons.
- Move installation issues to a drawer shown when Install button
is clicked (gh#agama-project#agama#1778).

-------------------------------------------------------------------
Fri Nov 15 16:48:44 UTC 2024 - Ladislav Slezák <[email protected]>

Expand Down
33 changes: 32 additions & 1 deletion web/src/assets/styles/app.scss
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,38 @@ button.remove-link:hover {
right: 0;
bottom: 0;
left: 0;
z-index: 999;
}
}
}

.agama-install-button {
padding: var(--pf-v5-global--spacer--sm) var(--pf-v5-global--spacer--md);
font-size: var(--fs-large);
}

.agama-issues-mark {
background: white;
width: 24px;
height: 24px;
border-radius: 24px;
top: -7px;
right: -7px;
position: absolute;
display: flex;
align-content: center;
justify-content: center;
}

.agama-issues-drawer-body {
padding: var(--pf-v5-global--spacer--lg);

h4 a {
text-decoration: underline;
font-weight: var(--fw-bold);
}

ul li.pf-m-info,
ul li.pf-m-warning {
--pf-v5-c-notification-drawer__list-item--before--BackgroundColor: none;
}
}
9 changes: 9 additions & 0 deletions web/src/assets/styles/patternfly-overrides.scss
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,12 @@
inline-size: 250px;
}
}

// A temporary workaround to fix "stacking contexts" problems with scroll and
// sticky page sections in Agama layout. It will not be needed when migrating to
// latest PF6 release, since the root of the problem has been addressed there by
// removing the DrawerContentBody from the Page component. See
// https://github.com/patternfly/patternfly/pull/7130 and related links
.pf-v5-c-drawer__body {
display: contents;
}
35 changes: 27 additions & 8 deletions web/src/components/core/InstallButton.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,23 @@ describe("InstallButton", () => {
);
});

it("renders nothing", () => {
it("renders additional information to warn users about found problems", () => {
const { container } = installerRender(<InstallButton />);
expect(container).toBeEmptyDOMElement();
const button = screen.getByRole("button", { name: /Install/ });
// An exlamation icon as visual mark
const icon = container.querySelector("svg");
expect(icon).toHaveAttribute("data-icon-name", "exclamation");
// An aria-label for users using an screen reader
within(button).getByLabelText(/Not possible with the current setup/);
});

it("triggers the onClickWithIssues callback without rendering the confirmation dialog", async () => {
const onClickWithIssuesFn = jest.fn();
const { user } = installerRender(<InstallButton onClickWithIssues={onClickWithIssuesFn} />);
const button = screen.getByRole("button", { name: /Install/ });
await user.click(button);
expect(onClickWithIssuesFn).toHaveBeenCalled();
await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument());
});
});

Expand All @@ -78,16 +92,21 @@ describe("InstallButton", () => {
mockIssuesList = new IssuesList([], [], [], []);
});

it("renders an Install button", () => {
installerRender(<InstallButton />);
screen.getByRole("button", { name: "Install" });
it("renders the button without any additional information", () => {
const { container } = installerRender(<InstallButton />);
const button = screen.getByRole("button", { name: "Install" });
// Renders nothing else
const icon = container.querySelector("svg");
expect(icon).toBeNull();
expect(within(button).queryByLabelText(/Not possible with the current setup/)).toBeNull();
});

it("renders a confirmation dialog when clicked", async () => {
const { user } = installerRender(<InstallButton />);
it("renders a confirmation dialog when clicked without triggering the onClickWithIssues callback", async () => {
const onClickWithIssuesFn = jest.fn();
const { user } = installerRender(<InstallButton onClickWithIssues={onClickWithIssuesFn} />);
const button = await screen.findByRole("button", { name: "Install" });
await user.click(button);

expect(onClickWithIssuesFn).not.toHaveBeenCalled();
screen.getByRole("dialog", { name: "Confirm Installation" });
});

Expand Down
33 changes: 25 additions & 8 deletions web/src/components/core/InstallButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,14 @@
*/

import React, { useState } from "react";

import { Button, ButtonProps, Stack } from "@patternfly/react-core";

import { Popup } from "~/components/core";
import { _ } from "~/i18n";
import { startInstallation } from "~/api/manager";
import { useAllIssues } from "~/queries/issues";
import { useLocation } from "react-router-dom";
import { PRODUCT, ROOT } from "~/routes/paths";
import { _ } from "~/i18n";
import { Icon } from "../layout";

/**
* List of paths where the InstallButton must not be shown.
Expand Down Expand Up @@ -81,26 +80,44 @@ according to the provided installation settings.",
* When clicked, it will ask for a confirmation before triggering the request
* for starting the installation.
*/
const InstallButton = (props: Omit<ButtonProps, "onClick">) => {
const InstallButton = (
props: Omit<ButtonProps, "onClick"> & { onClickWithIssues?: () => void },
) => {
const issues = useAllIssues();
const [isOpen, setIsOpen] = useState(false);
const location = useLocation();
const hasIssues = !issues.isEmpty;

if (!issues.isEmpty) return;
if (EXCLUDED_FROM.includes(location.pathname)) return;

const { onClickWithIssues, ...buttonProps } = props;
const open = async () => setIsOpen(true);
const close = () => setIsOpen(false);
const onAccept = () => {
close();
startInstallation();
};

// TRANSLATORS: The install button label
const buttonText = _("Install");
// TRANSLATORS: Accessible text included with the install button when there are issues
const withIssuesAriaLabel = _("Not possible with the current setup. Click to know more.");

return (
<>
<Button variant="primary" {...props} onClick={open}>
{/* TRANSLATORS: button label */}
{_("Install")}
<Button
variant="primary"
size="lg"
className="agama-install-button"
{...buttonProps}
onClick={hasIssues ? onClickWithIssues : open}
>
{buttonText}
{hasIssues && (
<div className="agama-issues-mark" aria-label={withIssuesAriaLabel}>
<Icon name="exclamation" size="xs" color="custom-color-300" />
</div>
)}
</Button>

{isOpen && <InstallConfirmationPopup onAccept={onAccept} onClose={close} />}
Expand Down
130 changes: 130 additions & 0 deletions web/src/components/core/IssuesDrawer.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* 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 the GNU General Public License as published by the Free
* Software Foundation; either version 2 of the License, or (at your option)
* any later version.
*
* 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, within } from "@testing-library/react";
import { installerRender } from "~/test-utils";
import { InstallationPhase } from "~/types/status";
import { IssuesList } from "~/types/issues";
import IssuesDrawer from "./IssuesDrawer";

let phase = InstallationPhase.Config;
let mockIssuesList: IssuesList;
const onCloseFn = jest.fn();

jest.mock("~/queries/issues", () => ({
...jest.requireActual("~/queries/issues"),
useAllIssues: () => mockIssuesList,
}));

jest.mock("~/queries/status", () => ({
useInstallerStatus: () => ({
phase,
}),
}));

const itRendersNothing = () =>
it("renders nothing", () => {
const { container } = installerRender(<IssuesDrawer onClose={onCloseFn} />);
expect(container).toBeEmptyDOMElement();
});

describe("IssuesDrawer", () => {
describe("when there are no installation issues", () => {
beforeEach(() => {
mockIssuesList = new IssuesList([], [], [], []);
});

itRendersNothing();
});

describe("when there are installation issues", () => {
beforeEach(() => {
mockIssuesList = new IssuesList(
[],
[
{
description: "Software Fake Issue",
source: 0,
severity: 0,
details: "Software Fake Issue details",
},
],
[
{
description: "Storage Fake Issue 1",
source: 0,
severity: 0,
details: "Storage Fake Issue 1 details",
},
{
description: "Storage Fake Issue 2",
source: 0,
severity: 0,
details: "Storage Fake Issue 2 details",
},
],
[
{
description: "Users Fake Issue",
source: 0,
severity: 0,
details: "Users Fake Issue details",
},
],
);
});

it("renders the drawer with categorized issues linking to their scope", async () => {
const { user } = installerRender(<IssuesDrawer onClose={onCloseFn} />);

const softwareIssues = screen.getByRole("region", { name: "Software" });
const storageIssues = screen.getByRole("region", { name: "Storage" });
const usersIssues = screen.getByRole("region", { name: "Users" });

const softwareLink = within(softwareIssues).getByRole("link", { name: "Software" });
expect(softwareLink).toHaveAttribute("href", "/software");
within(softwareIssues).getByText("Software Fake Issue");

const storageLink = within(storageIssues).getByRole("link", { name: "Storage" });
expect(storageLink).toHaveAttribute("href", "/storage");
within(storageIssues).getByText("Storage Fake Issue 1");
within(storageIssues).getByText("Storage Fake Issue 2");

const usersLink = within(usersIssues).getByRole("link", { name: "Users" });
expect(usersLink).toHaveAttribute("href", "/users");
within(usersIssues).getByText("Users Fake Issue");

const closeButton = screen.getByRole("button", { name: "Close" });
await user.click(closeButton);
expect(onCloseFn).toHaveBeenCalled();
});

describe("at install phase", () => {
beforeEach(() => {
phase = InstallationPhase.Install;
});

itRendersNothing();
});
});
});
Loading

0 comments on commit e721f35

Please sign in to comment.