Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breakpoint redfinition at compiled CSS "optimization" time #17629

Closed
garrett opened this issue Aug 8, 2022 · 1 comment
Closed

Breakpoint redfinition at compiled CSS "optimization" time #17629

garrett opened this issue Aug 8, 2022 · 1 comment
Assignees

Comments

@garrett
Copy link
Member

garrett commented Aug 8, 2022

Background: Cockpit uses iframes and PatternFly completely ignores iframes. Therefore, everything outside of the iframe eats away pixels from the document inside the iframe. So PatternFly's assumption of the navigation isn't taken into account when the navigation is outside the iframe (like in Cockpit) instead of inside (like with OpenShift and what PatternFly does by default).

Therefore: We've been jumping through hoops and redefining the breakpoint variables at the SCSS compilation level.

This still has several problems:

  1. Not all of the in-use breakpoints are redefined across all of Cockpit. (In other words: We miss some of the breakpoints for some of the widgets sometimes.)
  2. SCSS imports via JSX are using the wrong PF breakpoints.
  3. CSS imports via JSX are using the wrong PF breakpoints.
  4. To redefine a breakpoint, we need to include specific PF SCSS files inside our own local SCSS files. This sometimes causes (intentional, but still bad) duplications.
  5. We have to manually think about and include SCSS with our own breakpoint redfinitions.
  6. External projects (cockpit-*, including our own, such as Machines, Podman, etc.) are sometimes not using the correct breakpoints.

I've brought up this problem with the PatternFly team multiple times over the past several years, including my latest attempt from last year with a table, screenshots, and some example code @ patternfly/patternfly#4511... without any traction.

So, unless PatternFly finally fixes this in their codebase, we currently only have 4 options:

  • Do nothing and have broken UI in several places and (intentionally) duplicated code, which does get in the way of dark mode and page load + render times.
  • Change PatternFly compilation, including the CSS, to statically compile the correct breakpoints. (This wouldn't work correctly for external sites.)
  • Fork PatternFly with our changes (and manually update our fork, rebasing breakpoints and any other changes).
  • Lightly rewrite the CSS after everything is compiled.

I think the lesser evil of these 4 options is to rewrite* the CSS, especially as we're drastically rewriting our CSS with a "CSS optimizer" anyway (and that definitely has broken things in production code).

(* To be clear: Here, I mean have it programmatically rewritten at build time, like regular expressions.)

Our changes would amount to looking for a line that begins with @media and change the following px sizes:

  PF4 default Cockpit breakpoint
xs 0 0
sm 576 236
md 768 428
lg 992 652
xl 1200 860
2xl 1450 1110

So, sm would be s/576px/236px/g for @media lines, md would be s/768px/428/g, and so on.

The end result is that we wouldn't have to care about breakpoints while writing JSX and SCSS code. Everything should "just work" from the viewpoint of development and what Cockpit's end users see.

(Of course, we'd want to make this adaptable for cockpit-* projects too.)

@garrett
Copy link
Member Author

garrett commented Aug 8, 2022

General idea

What I'd imagine this to look like:

  1. Exclude shell.scss (which is the outer frame) and nav.scss (also used for the shell), as these are specifically outside the iframe
  2. Also exclude login.css (as this is outside of the Cockpit app)
  3. Look for media queries with lines that start with @media
  4. Rewrite the matching px values

Possible solutions

WebPack processAssets

Part of WebPack >= 5

As seen in https://stackoverflow.com/a/65529189
WebPack docs: https://webpack.js.org/api/compilation-hooks/#additional-assets

content-replace-webpack-plugin

Last release: 2020

https://www.npmjs.com/package/content-replace-webpack-plugin
https://github.com/ali322/content-replace-webpack-plugin

webpack-plugin-replace

Last release: 2019

https://www.npmjs.com/package/webpack-plugin-replace
https://github.com/lukeed/webpack-plugin-replace

css-rewrite-webpack-plugin

Last release: 2016

I guess this is specific to CSS? We probably don't need it to be specific, really.

https://www.npmjs.com/package/css-rewrite-webpack-plugin
https://github.com/hilongjw/css-rewrite-webpack-plugin

KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 2, 2023
Esbuild's main configuration now lives in build.js

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 3, 2023
Esbuild's main configuration now lives in build.js

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 3, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 3, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 3, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 3, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 8, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 8, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 15, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 15, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 15, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 15, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 16, 2023
Esbuild's main configuration now lives in build.js

TODO:
[ ] Fix LICENSE parsing for debian

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 16, 2023
Esbuild's main configuration now lives in build.js

Fixes cockpit-project#17629
KKoukiou added a commit to KKoukiou/cockpit that referenced this issue Mar 17, 2023
Esbuild's main configuration now lives in build.js

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 21, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 21, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 22, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 22, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 23, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 23, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 23, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 23, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 23, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 23, 2023
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 24, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 24, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 25, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 26, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 26, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 26, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Implement our own watch mode due to the deficiencies of esbuild's [1].
This is similar to podman's [2], but with recursive watching.

Because the approach for redefining breakpoints changed, update pixel
tests for the spacing noise.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629

[1] evanw/esbuild#1204 (comment)
[2] cockpit-project/cockpit-podman#1243
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 27, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Implement our own watch mode due to the deficiencies of esbuild's [1].
This is similar to podman's [2], but with recursive watching.

Because the approach for redefining breakpoints changed, update pixel
tests for the spacing noise.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629

[1] evanw/esbuild#1204 (comment)
[2] cockpit-project/cockpit-podman#1243
martinpitt pushed a commit to KKoukiou/cockpit that referenced this issue Mar 27, 2023
We don't need the `$ESLINT`/`$STYLELINT` env variables any more to
coordinate between build.js and webpack-config.js, as everything is in
build.js now. Update HACKING.md accordingly.

Implement our own watch mode due to the deficiencies of esbuild's [1].
This is similar to podman's [2], but with recursive watching.

Because the approach for redefining breakpoints changed, update pixel
tests for the spacing noise.

Co-Authored-By: Martin Pitt <mpitt@redhat.com>

Fixes cockpit-project#17629

[1] evanw/esbuild#1204 (comment)
[2] cockpit-project/cockpit-podman#1243
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants