Skip to content

Commit

Permalink
fix(web): ensure question are always rendered (#1825)
Browse files Browse the repository at this point in the history
Rendering questions across the app no longer works as expected after
#1690.

Commit f10153b changed how the content
outside the _installation settings context_ is handled, navigating to
its own path instead of rendering as part of `App` component.

_Mounting_ `Questions` component in `Layout` should restore the previous
behavior and fix #1820.
  • Loading branch information
dgdavid authored Dec 10, 2024
2 parents 6634543 + 70025e3 commit 96c2eb6
Show file tree
Hide file tree
Showing 10 changed files with 253 additions and 105 deletions.
6 changes: 6 additions & 0 deletions web/package/agama-web-ui.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Tue Dec 10 14:43:08 UTC 2024 - David Diaz <[email protected]>

- Restore the rendering of questions throughout the app
(gh#agama-project/agama#1820)

-------------------------------------------------------------------
Mon Dec 9 14:10:44 UTC 2024 - David Diaz <[email protected]>

Expand Down
1 change: 0 additions & 1 deletion web/src/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ jest.mock("~/context/installer", () => ({

// Mock some components,
// See https://www.chakshunyu.com/blog/how-to-mock-a-react-component-in-jest/#default-export
jest.mock("~/components/questions/Questions", () => () => <div>Questions Mock</div>);
jest.mock("~/components/layout/Loading", () => () => <div>Loading Mock</div>);
jest.mock("~/components/product/ProductSelectionProgress", () => () => <div>Product progress</div>);

Expand Down
25 changes: 5 additions & 20 deletions web/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@
import React from "react";
import { Navigate, Outlet, useLocation } from "react-router-dom";
import { ServerError } from "~/components/core";
import { Loading, PlainLayout } from "~/components/layout";
import { Questions } from "~/components/questions";
import { Loading } from "~/components/layout";
import { useInstallerL10n } from "~/context/installerL10n";
import { useInstallerClientStatus } from "~/context/installer";
import { useProduct, useProductChanges } from "~/queries/software";
Expand Down Expand Up @@ -54,12 +53,7 @@ function App() {
useDeprecatedChanges();

const Content = () => {
if (error)
return (
<PlainLayout>
<ServerError />
</PlainLayout>
);
if (error) return <ServerError />;

if (phase === InstallationPhase.Install && isBusy) {
return <Navigate to={ROOT.installationProgress} />;
Expand All @@ -69,14 +63,10 @@ function App() {
return <Navigate to={ROOT.installationFinished} />;
}

if (!products || !connected) return <Loading />;
if (!products || !connected) return <Loading useLayout />;

if (phase === InstallationPhase.Startup && isBusy) {
return (
<PlainLayout>
<Loading />
</PlainLayout>
);
return <Loading useLayout />;
}

if (selectedProduct === undefined && location.pathname !== PRODUCT.root) {
Expand All @@ -102,12 +92,7 @@ function App() {

if (!language) return null;

return (
<>
<Content />
<Questions />
</>
);
return <Content />;
}

export default App;
22 changes: 21 additions & 1 deletion web/src/components/core/ServerError.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,29 @@ import { plainRender } from "~/test-utils";
import * as utils from "~/utils";
import { ServerError } from "~/components/core";

jest.mock("~/components/layout/Header", () => () => <div>Header Mock</div>);
jest.mock("~/components/layout/Sidebar", () => () => <div>Sidebar Mock</div>);
jest.mock("~/components/layout/Layout", () => {
const layout = jest.requireActual("~/components/layout/Layout");
const OriginalPlainLayout = layout.Plain;

return {
...layout,
Plain: ({ ...props }) => (
<>
<div>PlainLayout Mock</div>
<OriginalPlainLayout {...props} />
</>
),
};
});

describe("ServerError", () => {
it("includes a generic server problem message", () => {
it("wraps a generic server problem message into a plain layout with neither, header nor sidebar", () => {
plainRender(<ServerError />);
expect(screen.queryByText("Header Mock")).toBeNull();
expect(screen.queryByText("Sidebar Mock")).toBeNull();
screen.getByText("PlainLayout Mock");
screen.getByText(/Cannot connect to Agama server/i);
});

Expand Down
38 changes: 20 additions & 18 deletions web/src/components/core/ServerError.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
EmptyStateBody,
EmptyStateHeader,
} from "@patternfly/react-core";
import { Center, Icon } from "~/components/layout";
import { Center, Icon, PlainLayout } from "~/components/layout";
import { Page } from "~/components/core";
import { _ } from "~/i18n";
import { locationReload } from "~/utils";
Expand All @@ -36,24 +36,26 @@ const ErrorIcon = () => <Icon name="error" className="icon-xxxl" />;

function ServerError() {
return (
<Page>
<Page.Content>
<Center>
<EmptyState variant="xl">
<EmptyStateHeader
titleText={_("Cannot connect to Agama server")}
headingLevel="h2"
icon={<EmptyStateIcon icon={ErrorIcon} />}
/>
<EmptyStateBody>{_("Please, check whether it is running.")}</EmptyStateBody>
</EmptyState>
</Center>
</Page.Content>
<PlainLayout mountHeader={false} mountSidebar={false}>
<Page>
<Page.Content>
<Center>
<EmptyState variant="xl">
<EmptyStateHeader
titleText={_("Cannot connect to Agama server")}
headingLevel="h2"
icon={<EmptyStateIcon icon={ErrorIcon} />}
/>
<EmptyStateBody>{_("Please, check whether it is running.")}</EmptyStateBody>
</EmptyState>
</Center>
</Page.Content>

<Page.Actions>
<Page.Action onClick={locationReload}>{_("Reload")}</Page.Action>
</Page.Actions>
</Page>
<Page.Actions>
<Page.Action onClick={locationReload}>{_("Reload")}</Page.Action>
</Page.Actions>
</Page>
</PlainLayout>
);
}

Expand Down
80 changes: 80 additions & 0 deletions web/src/components/layout/Layout.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* 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 } from "@testing-library/react";
import { plainRender } from "~/test-utils";
import Layout from "./Layout";

jest.mock("~/components/layout/Header", () => () => <div>Header Mock</div>);
jest.mock("~/components/layout/Sidebar", () => () => <div>Sidebar Mock</div>);
jest.mock("~/components/core/IssuesDrawer", () => () => <div>IssuesDrawer Mock</div>);
jest.mock("~/components/questions/Questions", () => () => <div>Questions Mock</div>);

describe("Layout", () => {
it("renders a page with header and sidebar by default", () => {
plainRender(<Layout />);
screen.getByText("Header Mock");
screen.getByText("Sidebar Mock");
});

it("does not render the header when mountHeader=false", () => {
plainRender(<Layout mountHeader={false} />);
expect(screen.queryByText("Header Mock")).toBeNull();
});

it("does not render the sidebar when mountSidebar=false", () => {
plainRender(<Layout mountSidebar={false} />);
expect(screen.queryByText("Sidebar Mock")).toBeNull();
});

describe("when children are not given", () => {
it("renders router <Outlet />", () => {
plainRender(<Layout />);
// NOTE: react-router-dom/Outlet is mock at src/test-utils
screen.getByText("Outlet Content");
});

it("renders the questions component", () => {
plainRender(<Layout />);
screen.getByText("Questions Mock");
});
});

describe("when children are given", () => {
it("renders children instead of router <Outlet />", () => {
plainRender(
<Layout>
<button>Dummy testing button</button>
</Layout>,
);
screen.getByRole("button", { name: "Dummy testing button" });
// NOTE: react-router-dom/Outlet is mock at src/test-utils
expect(screen.queryByText("Outlet Content")).toBeNull();
});

it("renders the questions component", () => {
plainRender(<Layout />);
screen.getByText("Questions Mock");
});
});
});
40 changes: 22 additions & 18 deletions web/src/components/layout/Layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@
import React, { Suspense, useState } from "react";
import { Outlet } from "react-router-dom";
import { Page } from "@patternfly/react-core";
import { Questions } from "~/components/questions";
import Header, { HeaderProps } from "~/components/layout/Header";
import { Loading, Sidebar } from "~/components/layout";
import { IssuesDrawer } from "~/components/core";

type LayoutProps = React.PropsWithChildren<{
export type LayoutProps = React.PropsWithChildren<{
mountHeader?: boolean;
mountSidebar?: boolean;
headerOptions?: HeaderProps;
Expand All @@ -50,23 +51,26 @@ const Layout = ({
const toggleIssuesDrawer = () => setIssuesDrawerVisible(!issuesDrawerVisible);

return (
<Page
isManagedSidebar
header={
mountHeader && (
<Header
showSidebarToggle={mountSidebar}
toggleIssuesDrawer={toggleIssuesDrawer}
{...headerOptions}
/>
)
}
sidebar={mountSidebar && <Sidebar />}
notificationDrawer={<IssuesDrawer onClose={closeIssuesDrawer} />}
isNotificationDrawerExpanded={issuesDrawerVisible}
>
<Suspense fallback={<Loading />}>{children || <Outlet />}</Suspense>
</Page>
<>
<Page
isManagedSidebar
header={
mountHeader && (
<Header
showSidebarToggle={mountSidebar}
toggleIssuesDrawer={toggleIssuesDrawer}
{...headerOptions}
/>
)
}
sidebar={mountSidebar && <Sidebar />}
notificationDrawer={<IssuesDrawer onClose={closeIssuesDrawer} />}
isNotificationDrawerExpanded={issuesDrawerVisible}
>
<Suspense fallback={<Loading />}>{children || <Outlet />}</Suspense>
</Page>
<Questions />
</>
);
};

Expand Down
36 changes: 0 additions & 36 deletions web/src/components/layout/Loading.test.jsx

This file was deleted.

Loading

0 comments on commit 96c2eb6

Please sign in to comment.