css: Duplicated PF imports

[garrett]

trafficstars

Cockpit still has some issues with duplicated CSS.

Some of these are due to imports in files that have been imported. Other issues are caused by using PatternFly components and also bringing in CSS. This issue is specifically about the latter, where components are pulled in via JSX and their CSS is also imported in SCSS files.

As an example, I took care of one in networking @ https://github.com/cockpit-project/cockpit/pull/17612. But I've found more files that could be affected.

This ag (silver searcher, like grep) finds:

$ ag "@import.*patternfly.*comp"

pkg/storaged/storage.scss
22:@import "@patternfly/patternfly/components/Card/card.scss";

pkg/lib/cockpit-components-table.scss
2:@import "@patternfly/patternfly/components/Table/table.scss";
3:@import "@patternfly/patternfly/components/Table/table-grid.scss";

pkg/lib/page.scss
4:@import "@patternfly/patternfly/components/Page/page.scss";

pkg/metrics/metrics.scss
4:@import "@patternfly/patternfly/components/Table/table.scss";
5:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";

pkg/shell/shell.scss
25:@import "@patternfly/patternfly/components/Button/button.css";

pkg/shell/nav.scss
2:@import "../../node_modules/@patternfly/patternfly/components/Toolbar/toolbar.scss";

pkg/systemd/services/services.scss
3:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";

pkg/systemd/terminal.scss
4:@import "@patternfly/patternfly/components/Button/button.css";
5:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";

pkg/systemd/logs.scss
5:@import "@patternfly/patternfly/components/FormControl/form-control.scss";
6:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";

pkg/systemd/overview.scss
5:@import "@patternfly/patternfly/components/Table/table.scss";

pkg/users/users.scss
7:@import "@patternfly/patternfly/components/DataList/data-list.scss";
8:@import "@patternfly/patternfly/components/Form/form.scss";

This usually happens when we used PF without React for PatternFly-HTML versus PatternFly-React and forgot to remove the custom CSS when switching to React, as PatternFly-React components pull in the necessary CSS when importing the components in JSX.

Some of these, such as shell, may be using HTML versions of PatternFly components instead of React, so some of these imports may be necessary, so we can't just remove all of these.

[jelly]

pkg/users/users.scss
7:@import "@patternfly/patternfly/components/DataList/data-list.scss";
8:@import "@patternfly/patternfly/components/Form/form.scss";

Removing this requires rewriting authorized-keys-panel to use our PF/Our data list component instead of writing div's with PF classes.

pkg/storaged/storage.scss
22:@import "@patternfly/patternfly/components/Card/card.scss";

Introduced in b16867fa73ea0f2c2a9af1670c761800d77383f3 can't tell if it's still required.

[garrett]

In today's deep dive into our codebase, I've moved some CSS from JSX to SCSS files. At least in some of the cases, the resulting compiled CSS appears to have removed some duplication: https://github.com/cockpit-project/cockpit/pull/17642

[martinpitt]

We still have a bunch of these:

❱❱❱ git grep "@import.*patternfly.*comp"

These hits are OK, their component/page does not import the React PF component:

pkg/lib/cockpit-components-table.scss:@import "@patternfly/patternfly/components/Table/table.scss";
pkg/lib/cockpit-components-table.scss:@import "@patternfly/patternfly/components/Table/table-grid.scss";
pkg/shell/shell.scss:@import "@patternfly/patternfly/components/Select/select.css";
pkg/storaged/storage.scss:@import "@patternfly/patternfly/components/Progress/progress.scss";
pkg/systemd/overview.scss:@import "@patternfly/patternfly/components/Table/table.scss";
pkg/users/users.scss:@import "@patternfly/patternfly/components/DataList/data-list.scss";

Duplication, page already imports PF component:

pkg/shell/nav.scss:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
pkg/networkmanager/networking.scss:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
pkg/storaged/storage.scss:@import "@patternfly/patternfly/components/Card/card.scss";
pkg/systemd/logs.scss:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
pkg/systemd/services/services.scss:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
pkg/systemd/terminal.scss:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
pkg/users/users.scss:@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";

This is messy -- all pages import page.scss, but only some import Page. We have to rely on automatic de-duplication here:

pkg/lib/page.scss:@import "@patternfly/patternfly/components/Page/page.scss";

unchecked:

For example, pkg/users/accounts-list.js already does import { Toolbar } from '@patternfly/react-core', so this looks like an example of duplicated CSS. However, dist/users/users.css.gz does not actually duplicate this -- there is just one instance of --pf-c-toolbar__expandable-content--Display: or .pf-c-chip-group:last-child. So I think that's the same case as in #17642, imports from scss files seem to be handled fine.

Same with dist/systemd/logs.css.gz , it only has one copy of the toolbar.scss rules, even though pkg/systemd/logs.jsx and pkg/systemd/logs.scss both import it.

OTOH, the select.scss import in pkg/shell/shell.scss is necessary because pkg/shell/hosts.jsx intentionally uses HTML instead of the React component.

Nevertheless, there are still some imports here which we could probably drop.

[martinpitt]

Hmm, interesting.. I tried

--- pkg/shell/nav.scss
+++ pkg/shell/nav.scss
@@ -1,5 +1,4 @@
 @import "global-variables";
-@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
 
 /* Navigation base, used for both desktop & mobile navigation */
 

and now dist/shell/index.css.gz is entirely missing the toolbar.scss rules, and the toolbar is broken:

this isn't at all obvious -- pkg/shell/topnav.jsx imports Toolbar from pf-react.

Firewall "Add services" normally looks like this:

But trying to remove the alleged duplication with

--- pkg/networkmanager/networking.scss
+++ pkg/networkmanager/networking.scss
@@ -1,8 +1,6 @@
 @use "ct-card";
 @use "page";
 @import "global-variables";
-// Needed for Firewall's add services dialog's filter layout
-@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
 
 #firewall,
 #network-page {

breaks the "filter services" input:

This really feels like a PatternFly bug/issue. Importing a component should include its CSS changes, and pkg/networkmanager/networking.scss does not refer to any variable set in toolbar.scss.

Anyway, as we apparently don't get duplication in the resulting CSS files, and these imports are necessary, I abandon this attempt. Closing this issue. @garrett , if you disagree and still find some duplication, please yell and reopen. Thanks!

[jelly]

Hmm, interesting.. I tried

--- pkg/shell/nav.scss
+++ pkg/shell/nav.scss
@@ -1,5 +1,4 @@
 @import "global-variables";
-@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
 
 /* Navigation base, used for both desktop & mobile navigation */
 

and now dist/shell/index.css.gz is entirely missing the toolbar.scss rules, and the toolbar is broken:

this isn't at all obvious -- pkg/shell/topnav.jsx imports Toolbar from pf-react.

Firewall "Add services" normally looks like this:

But trying to remove the alleged duplication with

--- pkg/networkmanager/networking.scss
+++ pkg/networkmanager/networking.scss
@@ -1,8 +1,6 @@
 @use "ct-card";
 @use "page";
 @import "global-variables";
-// Needed for Firewall's add services dialog's filter layout
-@import "@patternfly/patternfly/components/Toolbar/toolbar.scss";
 
 #firewall,
 #network-page {

breaks the "filter services" input:

This really feels like a PatternFly bug/issue. Importing a component should include its CSS changes, and pkg/networkmanager/networking.scss does not refer to any variable set in toolbar.scss.

Anyway, as we apparently don't get duplication in the resulting CSS files, and these imports are necessary, I abandon this attempt. Closing this issue. @garrett , if you disagree and still find some duplication, please yell and reopen. Thanks!

The Toolbar import is not really duplicated as it's only imported in a page specific css, not in for example pkg/lib so it's not too bad. But yeah it feels like a PF issue that the styling is not there.

The example in PF seems to work fine https://www.patternfly.org/v4/components/toolbar

[jelly]

Ah woops, this is due to the breakpoint redefining in webpack.js. So not related to PF.