Skip to content

Commit

Permalink
[web] Improve core/Page component and change how Agama layout is hand…
Browse files Browse the repository at this point in the history
…led (#925)

## The main change

As the project evolves, some decisions have been taken in regard to the
elements' placement in the UI. Although nothing is set in stone, the
truth is that these changes have been steady for several months already.
However, the UI internals haven't been updated accordingly to them,
keeping a complexity that comes from the early days of Agama when
everything was less defined.

It is the case of the core/Layout component and its slots, managed by
the react-teleporter dependency. They were there because of the need to
_influence_ the layout from its children, which fortunately is no longer
the case. E.g., it was needed to "teleport" a navigation link from the
page or page's section to the sidebar, something that was called
"contextual actions". Later, we realized that it was better to have a
kind of page menu accessible from the header. What is more, the Sidebar
ended up being relegated for holding only Agama stuff, which makes it
kind of _static_.

Thus, core/Layout and its _teleportable_ slots does not make sense
anymore. At this time they just provide a worst DX and a no longer
justifiable use of React Portals. Moreover, since core/Page has become a
cornerstone while it comes to Agama UI, it looks reasonable to build the
layout from there. Doesn't matter if in the future it's needed to manage
different page layouts, they can be extracted and _injected_ via
core/Page props or using another technique. About performance, it turns
out that _rendering everything_ when user navigates from one page to
another does not have a so negative impact. Remember that React (the UI
library Agama is using) [only changes the DOM nodes if there’s a
difference between
renders](https://react.dev/learn/render-and-commit#step-3-react-commits-changes-to-the-dom).
Furthermore, asking for _re-renders_ on user navigation is not as
different from a basic client/server web application: when the user
navigates, the server sends back new content and usually there is no
performance penalty since browsers know how to do their work efficiently
even when for this example they have to fully _updates_ the
[DOM](https://dom.spec.whatwg.org/). In any case, _by good luck_ we're
constantly trying to keep the DOM size as minimum as possible, see
https://web.dev/articles/dom-size-and-interactivity.


## Other things

Additionally, some other changes have been done as part of this set of
changes, some of them needed to accomplish the main goal described
above. Take a look at the commits to know more.

## Notes

From #925 (comment):

> **A note about using `data-` attributes for styling**
> 
> I started using them for _exceptions_, as [suggested by
CUBE](https://cube.fyi/exception.html#exception) methodology which I'd
like to follow (WIP). But lately I found myself using them more to make
styles easier to understand and, at some level, improve DX.
> 
> I'm aware that CSS _is_ mostly about classes selectors and they are
likely to be more performant than data- attributes when used for
styling. Though I haven't tested it myself, the potential performance
penalty appears to be insignificant. See
https://webdev.tips/styling-using-data-attributes#heading-performance-considerations,
https://ecss.benfrain.com/appendix1.html, and
https://gomakethings.com/how-performant-are-data-attributes-as-selectors/
> 
> Keeping a balance is the key here. Making use of them when it seems
reasonable, and most importantly, trying **not to abuse them**. It's
actually **my** goal to **use as few selectors as possible**, regardless
of their type.
> 
> Of course, I can be wrong when taking a decision of adding or removing
a selector. And that's why I'm adding this note, to let you know what's
the main intention behind my _CSS movements_ and to encourage you to
join me in the joy of exploring how to make the Agama CSS great and
**minimal** at the same time :)
> 
> Also, I've to say that I'm not afraid to make mistakes here. They give
me/us the opportunity to learn and improve. Furthermore, CSS **is easy
to change and fix**.
> 
> BTW, when looking for evidences about data- selectors performance I
found an article talking about [Class-less CSS
architecture](https://jacobxperez.github.io/blog/post/css/class-less-architecture/)
:innocent:

---

Related to https://trello.com/c/h367wPYP (internal link)
  • Loading branch information
imobachgs authored Dec 21, 2023
2 parents 20c3aa3 + d2667b1 commit fb910c4
Show file tree
Hide file tree
Showing 86 changed files with 1,401 additions and 1,184 deletions.
1 change: 1 addition & 0 deletions web/.svgrrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{
"icon": 28,
"plugins": ["@svgr/plugin-jsx"]
}
4 changes: 2 additions & 2 deletions web/__mocks__/svg.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="48" height="48" {...props}>
<rect x="0" y="0" width="48" height="48" />
<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="28" height="28" {...props}>
<rect x="0" y="0" width="28" height="28" />
</svg>
);
6 changes: 4 additions & 2 deletions web/cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@
"ipaddr",
"iscsi",
"jdoe",
"keymap",
"keymaps",
"keymap",
"keymaps",
"libyui",
"lldp",
"localdomain",
Expand All @@ -68,13 +68,15 @@
"subvol",
"subvolume",
"subvolumes",
"svgrrc",
"teleporter",
"testfile",
"testsuite",
"textinput",
"tkip",
"udev",
"wwpn",
"xxxs",
"zfcp"
]
}
Expand Down
14 changes: 0 additions & 14 deletions web/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
7 changes: 7 additions & 0 deletions web/package/cockpit-agama.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
-------------------------------------------------------------------
Thu Dec 21 13:36:46 UTC 2023 - David Diaz <dgonzalez@suse.com>

- 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.

-------------------------------------------------------------------
Thu Dec 21 11:18:37 UTC 2023 - Ancor Gonzalez Sosa <ancor@suse.com>

Expand Down
45 changes: 3 additions & 42 deletions web/src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,13 @@
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 { InstallerKeymapSwitcher, InstallerLocaleSwitcher } from "./components/l10n";
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.
Expand Down Expand Up @@ -103,34 +91,7 @@ function App() {
};

return (
<>
<Sidebar>
<div className="flex-stack">
<IssuesLink />
<Disclosure label={_("Diagnostic tools")} data-keep-sidebar-open>
<ShowLogButton />
<LogsButton data-keep-sidebar-open="true" />
<ShowTerminalButton />
</Disclosure>
<About />
</div>
<div className="full-width highlighted">
<div className="flex-stack">
<div className="locale-container">
<div><InstallerLocaleSwitcher /></div>
<div><InstallerKeymapSwitcher /></div>
</div>
</div>
</div>
</Sidebar>

<Layout>
{/* this is the name of the tool, do not translate it */}
{/* eslint-disable-next-line i18next/no-literal-string */}
<Title>Agama</Title>
{language && <Content />}
</Layout>
</>
<If condition={language} then={<Content />} />
);
}

Expand Down
15 changes: 9 additions & 6 deletions web/src/App.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -43,10 +44,9 @@ 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", () => <div>D-BusError Mock</div>);
jest.mock("~/components/questions/Questions", () => () => <div>Questions Mock</div>);
jest.mock("~/components/core/Installation", () => () => <div>Installation Mock</div>);
jest.mock("~/components/core/Sidebar", () => () => <div>Sidebar Mock</div>);
jest.mock("~/components/layout/Loading", () => () => <div>Loading Mock</div>);

// this object holds the mocked callbacks
const callbacks = {};
Expand All @@ -73,6 +73,10 @@ describe("App", () => {
l10n: {
locales: jest.fn().mockResolvedValue([["en_us", "English", "United States"]]),
getLocales: jest.fn().mockResolvedValue(["en_us"]),
timezones: jest.fn().mockResolvedValue([]),
getTimezone: jest.fn().mockResolvedValue("Europe/Berlin"),
keymaps: jest.fn().mockResolvedValue([]),
getKeymap: jest.fn().mockResolvedValue(undefined),
getUILocale: jest.fn().mockResolvedValue("en_us"),
setUILocale: jest.fn().mockResolvedValue("en_us"),
onTimezoneChange: jest.fn(),
Expand Down Expand Up @@ -100,7 +104,7 @@ describe("App", () => {

it("renders the Loading screen", async () => {
installerRender(<App />, { withL10n: true });
await screen.findByText(/Loading installation environment/);
await screen.findByText("Loading Mock");
});
});

Expand All @@ -112,7 +116,7 @@ describe("App", () => {

it("renders the Loading screen", async () => {
installerRender(<App />, { withL10n: true });
await screen.findByText(/Loading installation environment/);
await screen.findByText("Loading Mock");
});
});

Expand All @@ -124,8 +128,7 @@ describe("App", () => {

it("renders the Loading screen", async () => {
installerRender(<App />, { withL10n: true });

await screen.findByText(/Loading installation environment/);
await screen.findByText("Loading Mock");
});
});

Expand Down
Loading

0 comments on commit fb910c4

Please sign in to comment.