refactor(css): Cleaned up stylesheets#2622
Conversation
|
(Not a maintainer) I came here looking for an issue like this or I would have opened one myself. I'm happy to take a closer look later and give my five cents👋 |
alteixeira20
left a comment
There was a problem hiding this comment.
Thanks for continuing to work on this. I agree with using #2617 as the canonical CSS maintainability tracker, but this PR is not safe to merge in its current form.
Requesting changes because the current head is still not a behavior-preserving CSS extraction.
The merge from current dev resolved the previous freshness problem, but it updated static/style.css by 902 additions and 33 deletions without updating any of the extracted static/css/** files. The new CSS entry point is therefore already stale.
The imported bundle also is not equivalent to the original stylesheet. It has additional and missing declaration occurrences, duplicated reset/token sections, and 162 keyframe definitions versus 142 in static/style.css. That can change cascade, reset, token, and animation behavior.
There is also an offline-cache issue: index.html now loads /static/css/index.css, which imports 14 stylesheets, but static/sw.js still precaches only /static/style.css. The old stylesheet also remains in the repository.
Please rework this as a strictly mechanical, order-preserving split from the current dev stylesheet:
- preserve declaration content and cascade order;
- remove unintended duplication;
- demonstrate exact declaration/order parity;
- decide and document whether the old stylesheet should be removed;
- update the service-worker precache for the actual CSS dependency graph;
- provide durable desktop/mobile, dark/light, density, modal, hard-reload, and offline validation evidence.
Separately, the Conventional Commit title check is currently failing.
Token redesign, reset cleanup, visual changes, and broad deduplication should stay out of this initial extraction and be handled separately after a validated modular baseline exists.
|
Appreciate the reviews! I've read anything and I'll make the following changes:
This may take me a while, I estimate by the end of next week since I'm quite busy at the moment, I hope this can be my first major repo PR merge in my career though :) |
ad08b62 to
fe54d41
Compare
|
Finished adding the fixes, sorry it took me a while!
Here is a video showcasing in detail the CSS ui that remains unchanged, in the video i restart the docker container entirely, showing that the CSS refactor is in place in VS code (correct branch + changes), then go through each UI screen and showing that the UI behaviour remains unchanged: https://youtu.be/FWFtUtlOyEo I've removed styles.css accordingly as well Let me know if there's any further changes to be made |
|
I need a notification the second this is mergeable. :) |
Bring the modular-CSS refactor up to date with 402 commits of dev. Conflict resolution: - static/style.css (modify/delete): kept deleted. Redistributed dev's +3034/-256 line delta (151 hunks) into the modular static/css/* files, preserving cascade order. 129 hunks placed by exact-context patch; 22 dedup-adjacent hunks hand-placed into their feature files. - static/index.html: kept the modular <link> to css/index.css; carried dev's ?v=20260630mdfontsize cache-bust on app.js. - static/sw.js: kept the modular PRECACHE list; bumped CACHE_NAME to v345 (past dev's v344) so clients from either side refetch. Also: - tests/helpers/css_loader.py: restored the runtime-CSS loader the branch's test changes import but never committed (recovered from its .pyc). - tests/test_portal_dropdown_z_js.py: assert against the runtime CSS bundle instead of the removed static/style.css. Full suite: 4619 passed, 4 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@entitycs this should be mergeable now, I pulled from origin dev branch and adapted all new styles to updated stylesheet, preserved cascading order, and resolved any conflicts. Functionality is maintained of course since I haven't touched any files outside of the stylesheets and files that import them. Here's an updated demo of the CSS maintained: let me know if this is all good |
|
@crptk so far so good on my end. I'm battling some other conflicts (eg. emails) so I am not able to test everything yet. edit: It looks to be a little behind - a lot of recent changes got pushed apparently, but no biggie. You've designed it to make it easy to re-diff and re-parse out, so I'm doing just that to get the latest changes in. One pointed question though: Did you use 2-space tabs - or is that an artifact of something I did on my end? I only ask because the original diff I tried exploded in whitespace changes (the repo desperately needs a linting pass + config). notes as I merge changes:
originalEverything I can see - login/logout, chat, most modules, the background effects... all working as expected. Backend-driven tests pass.All existing & new tests related to css are passing for me. Though, I'm moving some tests from the style of 'loading node through python' into actual frontend tests (unit, and end-to-end w/ playwright). So I may have some overlap there, and would have replaced those existing tests. Eventually, in my svelte branch, every component/page will have its own style block <script>/* scripts */</script>
<div>/* html elements */</div>
<style>/* applicable styles */</style>Not having to pick from a 40K line file will make this much more manageable down the road. A big thanks for cutting that down. |
|
@entitycs Thanks! Glad this PR helps, I was somewhat surprised that nobody has made a PR about this earlier and I'm glad I was able to kick it off. About the whitespace issues, I believe it may be on my end. If you'd like I can fix it no worries! I'm just not sure if you already made changes to it and if I make a new commit it might produce more merge conflicts on your end |
|
@crptk don't worry about me. It's better handled by you, ofc, but additionally, because I know I made some personalized changes in style.css itself that could potentially pop up otherwise. Still, I'm just about done making the git difftool see the changes correctly (ie. make edits such that it can graph out the moved sections correctly). Out of the 40k lines or whatever it is, at least on my VSCode install, it gets confused between handfuls (or dozens) of identical batches of lines. If you want to compare, as patches against dev, I'm up to do that; otherwise, don't let me stop you :) |
Summary
Splits the ~34,000 lines of CSS in
static/styles.cssinto multiple smaller stylesheets organized by purpose under a newstatic/css/directory.An
index.cssfile imports all stylesheets and serves as the single entry point.It's important to note that no CSS has been changed, simply reorganizedand refactored into their own files. No UI changes are intended with this PR. The goal is to make the CSS easier to navigate and maintain, since working with a single 34k-line stylesheet has become increasingly difficult. The CSS itself could still benefit from further cleanup, but organizing it into smaller, purpose-based stylesheets should make future improvements much easier.
The new organization has been split like so:
Target branch
dev, notmain. All PRs land indev;mainis curated by the maintainer at each release. If your PR is onmainby accident, click "Edit" on this PR and change the base.Linked Issue
Fixes #2617
Type of Change
Checklist
devdocker compose uporuvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.How to Test
Visual / UI changes — REQUIRED if you touched anything that renders
Anything that changes what the UI looks like — buttons, icons, padding, colors, fonts, spacing, layout, CSS, HTML, SVG, or any
static/js/module that draws to the DOM — needs all of the following. PRs that change rendering without these WILL be closed.--red,--fg,--bg,--card,--border, etc.) — do not introduce new color values, font sizes, or spacing units.static/index.html) or plain text.Fira Code) for primary UI text. Don't override.Screenshots / clips
https://streamable.com/0zkqow