Skip to content

Replace Sass-built shared.css with plain CSS files - #928

Open
eduardosmaniotto wants to merge 7 commits into
MUnique:masterfrom
eduardosmaniotto:chore/migrate-to-css
Open

Replace Sass-built shared.css with plain CSS files#928
eduardosmaniotto wants to merge 7 commits into
MUnique:masterfrom
eduardosmaniotto:chore/migrate-to-css

Conversation

@eduardosmaniotto

@eduardosmaniotto eduardosmaniotto commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Removes the Sass build chain from MUnique.OpenMU.Web.Shared and replaces
the generated shared.css/shared.min.css bundle with plain, hand-maintainable
CSS files plus stock Bootstrap 5.3.8.

What changed

  • Web.Shared: deleted Styles/*.scss, Styles/bootstrap-5.3.8/,
    compilerconfig.json(.defaults), shared.css/shared.min.css;
    removed BuildWebCompiler2022 and the CompileSass target from the csproj
    (incl. a dead compilerconfig.json reference in ItemEditor).
  • New files in Web.Shared/wwwroot/css: bootstrap.min.css (stock 5.3.8),
    theme.css, navigation.css, main.css, forms.css, tables.css,
    common.css, log.css, map-editor.css. Custom rules were extracted 1:1
    from the last compiled output (mechanically verified
    declaration-for-declaration); the Tables.scss stagger loop
    (@keyframes FadeIn + 50 tr:nth-child(n) rules) was intentionally
    removed, and the Forms.scss/MapEditor.scss @extends were replaced
    with standalone/grouped selectors on Bootstrap default variables.
  • Bootstrap defaults adopted deliberately (no bootstrap-overrides.css):
    $primary: #1b6ec2 and the disabled validation icons are not restored.
    --omu-link-primary/-border were aligned to the Bootstrap defaults
    (#0d6efd/#0b5ed7, both modes) so ReconnectModal buttons match every
    other primary button. Trade-off: reskinning Bootstrap now needs explicit
    override rules instead of one Sass variable.
  • Exports.cs: serves the 10 stylesheets (bootstrap, open-iconic + 8 custom)
    as separate files instead of shared.css, in bootstrap → open-iconic →
    theme order. (Differs from the old bundle, where open-iconic's passthrough
    @import came first — no practical impact, the new order is the better one.)
  • AdminPanel + Map: new shared src/Web/ScopedCssBundleImports.targets
    (imported by both, runs after BundleScopedCssFiles, idempotent) that
    rewrites the SDK's root-relative bundle @import '_content/...' rules to
    base-relative ../../_content/..., since these bundles are served from
    their StaticWebAssetBasePath. Works at the site root and under
    UsePathBase (e.g. PATH_BASE=/map).
  • LogFiles page: fixed the colocated JS module path
    (./_content/..., same convention as PacketGrid); removed hardcoded
    table-light thead and bg-light card-header classes so the file list
    follows the selected theme; added form-control to the five class-less
    inputs in ConnectServerConfiguration.razor.
  • Dead toolchain references removed: BuildWebCompiler2022 version pin,
    Web Compiler docs requirement, .sass-cache gitignore entry.

@eduardosmaniotto
eduardosmaniotto marked this pull request as ready for review September 4, 2026 20:07
- Load the colocated LogFiles.razor.js module via its static web asset
  path (./_content/...), matching the convention used by ThemeSelector
  and MapEditor. The previous relative path resolved against the web
  root and 404'd, which broke log auto-scroll.
- Drop the hardcoded table-light thead class and bg-light card-header
  class so the file list follows the selected theme like all other
  admin panel tables and cards.

sven-n commented Sep 4, 2026

Copy link
Copy Markdown
Member

Review

Nice cleanup — dropping the BuildWebCompiler2022 / Dart-Sass split build chain removes a real
contributor-onboarding wart (Windows-only NuGet package + sass CLI on Linux + a 13k-line generated
file in git). The custom-CSS extraction is genuinely faithful, and I verified that mechanically
rather than by eyeballing: I parsed the custom tail of the old compiled shared.css
(everything from the :root, [data-bs-theme=light] token block onwards) and the concatenation of
the eight new hand-written files, then compared them selector-by-selector and
declaration-by-declaration. Zero differing declarations across all shared selectors. The only
deltas are the ones you already call out in the description (@keyframes FadeIn + the 50
tr:nth-child(n) rules dropped, and the .valid/.invalid.modified @extends rewritten). That's a
good sign for the mechanical part of the migration.

The concerns below are about the parts that are not 1:1.


1. Losing $primary: #1b6ec2 is a visible, app-wide regression (needs a decision)

The description says "no $primary/validation-icon overrides for now", but this is bigger than
"for now" suggests. Old compiled output vs. new stock bootstrap.min.css:

/* before */                        /* after */
.btn-primary {                      .btn-primary {
  --bs-btn-bg: #1b6ec2;               --bs-btn-bg: #0d6efd;
  --bs-btn-hover-bg: rgb(22.9,…);     --bs-btn-hover-bg: #0b5ed7;
}                                   }
--bs-link-color: #1b6ec2            --bs-link-color: #0d6efd
--bs-primary: #1b6ec2               --bs-primary: #0d6efd

btn-primary is used in ~20 razor files across AdminPanel, Map and Shared, so every primary button,
link, focus ring and active nav item changes colour. Worse, it becomes inconsistent: theme.css
still ships --omu-link-primary: #1b6ec2 / #2e86d6, and ReconnectModal.razor.css paints its
button from those tokens — so the reconnect modal will now be a visibly different blue from every
other primary button in the panel.

Also worth knowing before choosing the fix: overriding --bs-primary alone will not bring the old
colour back.
Bootstrap 5.3 bakes $primary into per-component literals at Sass compile time
(--bs-btn-bg, --bs-btn-hover-bg, --bs-btn-focus-shadow-rgb, .link-primary,
.text-bg-primary, --bs-link-color, …), so a CSS-only restore needs an explicit override block
covering those. That trade-off — you can no longer reskin Bootstrap by setting one variable — is the
main thing this PR gives up, and it deserves an explicit call rather than a "for now":

  • (a) accept the Bootstrap default blue, and update --omu-link-primary/-border in
    theme.css to match so the theme stays internally consistent; or
  • (b) add a small bootstrap-overrides.css after bootstrap.min.css that restores #1b6ec2
    for .btn-primary, .link-primary, --bs-link-color and the focus-ring rgb.

Either is fine, but please pick one in this PR rather than leaving the palette half-migrated.

2. @import '/_content/…' breaks under UsePathBase (the Map project especially)

The diagnosis in the MSBuild comment is right — CSS @import resolves against the stylesheet's
URL, not the document base, so the SDK's root-relative _content/… doubles up. But rewriting to a
root-absolute /_content/… hardcodes the assumption that the app is at the site root, and OpenMU
explicitly supports not being:

// src/Dapr/Common/Extensions.cs:244
var pathBase = Environment.GetEnvironmentVariable("PATH_BASE");app.UsePathBase(pathBase!.TrimEnd('/'));

That Dapr host is exactly the one that serves the Map Blazor app. With PATH_BASE=/map, the bundle
is served from /map/_content/MUnique.OpenMU.Web.Map/…, and the rewritten @import '/_content/…'
resolves to /_content/… — 404 again, just in a different deployment.

A relative rewrite fixes both cases, since the bundle always sits exactly two segments deep under
whatever base the app runs on:

var fixedText = text.Replace("@import '_content/", "@import '../../_content/");

/_content/<Project>/x.styles.css../..//, and /map/_content/<Project>/x.styles.css
../..//map/. Still idempotent (the second pass no longer matches @import '_content/), and
it survives fingerprinting, which changes the filename but not the directory.

Minor robustness note on the same replace: it only matches the single-quoted form. If the SDK ever
emits @import "_content/…" or a url() form, this silently no-ops and the 404 comes back with no
build signal. A regex over @import\s+['"]_content/ would fail less quietly.

3. The inline task is duplicated verbatim in two csproj files

AbsolutizeScopedCssBundleImports is copy-pasted identically (28 lines each) into
MUnique.OpenMU.Web.AdminPanel.csproj and MUnique.OpenMU.Web.Map.csproj. Given the fix above will
need editing in both places, please factor it into one shared file — e.g.
src/Web/ScopedCssBundleImports.targets imported by both, or a conditional block in
src/Directory.Build.props's sibling Directory.Build.targets. As a bonus, RoslynCodeTaskFactory
currently compiles the same task assembly twice per build.

Smaller point on the same target: the bundle path is reconstructed by hand as
$(IntermediateOutputPath)scopedcss/bundle/$(PackageId).styles.css. That's correct today, but it's
reverse-engineered from SDK internals and will break silently (the Exists condition just skips the
target) if the SDK moves it or if anyone sets PackageIdAssemblyName. Worth a comment saying
so, so a future 404 is easier to trace back here.

4. Dropping the FadeIn row-stagger is an unflagged UI change

Tables.scss generated 50 global tr:nth-child(n) rules with opacity: 0 +
animation-fill-mode: forwards, so every table in the app faded its rows in. That's now gone
entirely. I actually think removing it is defensible — a bare global tr selector applying to all
tables everywhere is heavy-handed, and 50 generated rules for it is a lot — but it's a deliberate
visual change hidden inside a "replace the build chain" PR. Please either keep it (it's ~10 lines of
plain CSS with a small nth-child set, or a uniform non-staggered fade) or say in the PR
title/description that table row animation is intentionally removed, so it doesn't surprise anyone
after merge.

5. Leftover from the @extend rewrite in forms.css

.auto-form-search-group .form-control, .auto-form-search-group .valid.modified {
  padding-left: 2.25rem;
}

The second selector is an artifact of extracting from the compiled output, where
.valid.modified { @extend .form-control; } had spliced that class into every .form-control
selector list in the file. Now that .valid.modified is a standalone two-declaration rule, that
selector is meaningless noise — drop it.

Related, and worth a quick visual check rather than a code change: old .valid.modified inherited
the entire .form-control box (display/width/padding/border/background) via @extend; the new
one only sets border-color. That's equivalent as long as every element Blazor stamps
valid modified onto already carries form-control — true for the standard AutoForm inputs, but
please confirm for any custom field components (MultiLookupField, the item/invasion tables) that
render their own inputs.

6. Validation icons are back on — inert today, but worth a note

Stock 5.3.8 re-enables what Site.scss disabled:

.is-valid, .was-validated .form-control:valid {
  padding-right: calc(1.5em + .75rem);
  background-image: url("data:image/svg+xml,…checkmark…");
}

I grepped the razor markup and found no uses of is-valid / is-invalid / was-validated, so
nothing regresses right now — Blazor's valid modified / invalid modified classes are handled by
your new rules and don't hit these selectors. But the project's deliberate "no validation icons"
decision is now undocumented and will silently reverse the first time someone writes
class="is-invalid". A one-line comment in forms.css recording the intent would preserve it.

7. The cascade-order claim isn't quite what shipped

The description says the files are served "in the original cascade order". The old order was
actually open-iconic first: sass left @import "open-iconic/…min.css" as a plain CSS
@import, which lands at line 7 of shared.css and is therefore applied before
everything else in that file, Bootstrap included. Exports.cs now emits bootstrap → open-iconic →
theme.

No practical impact — open-iconic only defines @font-face and .oi*, which Bootstrap doesn't
touch, and theme.css's [data-bs-theme=dark] .oi (0,2,0) outranks .oi (0,1,0) regardless of
order. The new order is arguably the better one. Just fix the description so the next person doesn't
trust an inaccurate invariant.

8. Leftovers from the removed Sass toolchain

Three references survive the removal:

  • src/Directory.Packages.props:10 still pins <PackageVersion Include="BuildWebCompiler2022" Version="1.14.15" /> — dead now.
  • docs-website/docs/getting-started/from-source.md:18-20 still tells contributors to install the
    Web Compiler 2022+
    VS extension "if you plan to edit SCSS files of the admin panel". Removing that line is arguably
    the best part of this PR — please don't leave it in.
  • .gitignore:133-134 # Web workbench (sass) / .sass-cache/ — harmless, but may as well go.

(-p:ci=true in dotnetcore.yml / azure-pipelines.yml is still needed — three other csproj files
use '$(ci)'!='true' — so nothing to do there.)

9. LogFiles.razor changes are out of scope and undescribed

The JS-module path fix is correct, and matches the convention elsewhere
(PacketGrid.razor.cs, AdminAuthenticationDefaults.AuthScriptPath) — the old
./Pages/LogFiles.razor.js couldn't resolve given AdminPanel's
StaticWebAssetBasePath = _content/$(MSBuildProjectName). Two things though:

  • Neither this nor the bg-light/table-light removals are mentioned in the PR description. The
    latter is a theme change (those Bootstrap classes stay light in dark mode, so removing them is
    probably right) — but a CSS-build-chain PR silently restyling the log page's card header and table
    head is the kind of thing that gets found later and blamed on the migration. Please list them, or
    split them into their own commit/PR.
  • Style: prefer a private const string next to the field, as PacketGrid.razor.cs:29 does, over
    an interpolated typeof(LogFiles).Assembly.GetName().Name string rebuilt on every
    OnAfterRenderAsync call. (Note Exports.GetPrefix() derives the same prefix from
    typeof(Exports).Namespace instead — three derivations of one constant is two too many.)

Nits

  • MUnique.OpenMU.Web.AdminPanel.csproj lines 64-72: two whitespace-only hunks (trailing-space
    churn inside the existing comment). Please revert so the diff is only the new target.
  • The UsingTask and the Target share the name AbsolutizeScopedCssBundleImports. Legal — MSBuild
    keeps separate namespaces — but confusing to read; _AbsolutizeScopedCssBundleImports for the
    target would help.
  • Ten blocking <link> elements instead of one. Fine for an admin panel over HTTP/2, and the eight
    custom files are only ~20 KB unminified, so I wouldn't re-add a bundler for it — just flagging it
    as a conscious trade. Worth noting src/Web/Map/App.razor:10 still emits raw @stylesheetSrc
    rather than @Assets[stylesheetSrc], so the Map host loses fingerprinting/cache-busting on all
    ten — pre-existing, but this PR multiplies the number of files affected.

Verdict: the direction is right and the extraction is provably faithful. I'd want #1 (pick a
primary-colour story) and #2 (../../_content/ so PATH_BASE deployments keep working) resolved
before merge; #3-#9 are cleanups that would make it a tidy change. Nothing here suggests reworking
the approach.

I reviewed statically — no .NET SDK in this environment — so the MSBuild target's runtime behaviour
and the visual diff of the rendered pages are unverified. Someone should build and eyeball the admin
panel in both light and dark mode, plus the Map under PATH_BASE, before merging.


Generated by Claude Code

- Decide the primary-color story: accept Bootstrap defaults and align
  --omu-link-primary/-border with them (#0d6efd/#0b5ed7, both modes),
  so ReconnectModal buttons match every other primary button.
- Rewrite scoped-css bundle @imports as base-relative
  (../../_content/...) so they resolve both at the site root and
  under UsePathBase (e.g. PATH_BASE=/map); match single- and
  double-quoted forms.
- Factor the bundle-import rewrite into shared
  src/Web/ScopedCssBundleImports.targets, imported by AdminPanel and
  Map, and revert whitespace churn in the AdminPanel workaround comment.
- forms.css: drop the leftover .valid.modified selector from the
  auto-form search rule; document that Bootstrap's own validation
  visuals stay intentionally unused. tables.css: note the row
  fade-in animation was intentionally removed.
- Remove dead Sass toolchain leftovers: BuildWebCompiler2022 version
  pin, Web Compiler docs requirement, .sass-cache gitignore entry.
- LogFiles.razor: use a private const for the JS module path, like
  PacketGrid does.

sven-n commented Sep 7, 2026

Copy link
Copy Markdown
Member

Re-review of efff494 + da8b6dc

Thanks — that's a thorough pass. Almost everything from the last round is addressed, and I re-ran the
same mechanical comparison against the new head: the eight custom files still match the old compiled
shared.css declaration-for-declaration, with the only deltas now being the four intentional
--omu-link-primary* values and the FadeIn keyframes + 50 tr:nth-child(n) rules. Nothing drifted
while fixing the rest. CI is green on all three checks.

Confirmed fixed: shared .targets file (#3), .valid.modified leftover (#5), the intent comments in
forms.css/tables.css (#4, #6), Directory.Packages.props + from-source.md + .gitignore (#8),
JsModulePath const (#9), target rename, and the csproj whitespace churn — AdminPanel.csproj is
now a clean 5-line diff against master.

Two things below: one new bug the hardening introduced, and one answer to a question I left open.


1. The new regex breaks the double-quoted case it was added to handle

var fixedText = Regex.Replace(text, @"@import\s+['""]_content/", "@import '../../_content/");

The pattern has no capture groups, so the match — including the opening quote — is replaced by a
literal that always ends in a single quote. For a double-quoted import that produces mismatched
quotes:

in : @import "_content/MUnique.OpenMU.Web.Shared/x.bundle.scp.css";
out: @import '../../_content/MUnique.OpenMU.Web.Shared/x.bundle.scp.css";
                                                                      ^ unterminated

That's an invalid @import, so the browser drops the rule entirely — the scoped styles just don't
load, with no build error. It's latent (the SDK emits single quotes today), but it makes the
double-quote path strictly worse than before the hardening: previously it was a silent no-op
leaving the doubled-path 404; now it's silently malformed CSS, and the comment right above claims
"Matching covers single- and double-quoted forms", so the next person won't look here.

Capturing the quote fixes it and keeps the whitespace intact:

var fixedText = Regex.Replace(text, @"@import(\s+)(['""])_content/", "@import$1$2../../_content/");

Verified against both quote styles and extra whitespace — output stays balanced, and it's still
idempotent ('../../_content/ no longer matches ['"]_content/). Consider \s* rather than \s+
while you're in there, so a minified @import'_content/… doesn't slip past.

2. Answering my own open question from #5: one page is affected

I asked you to check whether anything gets valid modified without also carrying form-control
I should have just checked. The old compiled output was:

.form-control, .valid.modified { display: block; width: 100%; padding: 0.375rem 0.75rem; … }

and src/Web/AdminPanel/Components/ConnectServer/ConnectServerConfiguration.razor has five inputs
with no class at all (lines 22, 29, 36, 46, 59) — <InputNumber id="patchVersionMajorInput" …/>,
<InputText @bind-Value=@this.Configuration.PatchAddress …/>, etc. Blazor stamps valid modified
on those once edited, so under the old CSS they were unstyled until you touched them and then
snapped to a full Bootstrap .form-control box. After this PR they stay browser-default.

Everything else is fine — InvasionSpawnTable's InputNumbers do carry class="form-control" (on
the following line, which is why my earlier grep looked worse than it was).

The old behaviour was really a bug — styling that appears only after the field is dirty — so please
don't restore the @extend. The fix is to add class="form-control" to those five inputs. That
whole component is bare <div>s where the rest of the panel uses input-group/form-label, so a
fuller cleanup is fair game for a separate PR; just the class would do here.

3. Still open: the PR description

The body is unchanged and now describes code that no longer exists:

  • "rewrites … to /_content/..." → it's ../../_content/ now.
  • "no $primary/validation-icon overrides for now" → you've since made the call (align --omu-*
    to the Bootstrap defaults), so "for now" undersells a decision that's now deliberate and documented
    in the CSS.
  • "in the original cascade order" (my Database Context issues when Items change their owner #7) — still inaccurate: the old order had open-iconic
    first, because sass left it as a plain CSS @import at line 7 of shared.css. No practical
    impact, the new order is fine; just don't leave a wrong invariant in the record.
  • The LogFiles.razor fixes, the bg-light/table-light removals, and the docs/.gitignore/
    Directory.Packages.props cleanups still aren't listed.

4. Worth one real build

Exists('$(IntermediateOutputPath)scopedcss/bundle/$(PackageId).styles.css') is now evaluated from
an imported file in a different directory. I'd expect it to still resolve against the importing
project's directory, but a green CI doesn't distinguish "target ran" from "condition was false and
the target was skipped" — which is exactly the silent-skip failure your own comment warns about.
Worth confirming once with -bl or by eyeballing the emitted bundle that the @import really comes
out as ../../_content/…, and ideally loading the Map under PATH_BASE since that's the case the
rewrite was changed for.


Verdict: #1 is the only thing I'd hold merge for, and it's a one-line change. #2 is a small
follow-up on a page this PR legitimately changes. #3 and #4 are bookkeeping and verification.
Still a static review here — no .NET SDK in this environment — so the rendered output remains
unverified.


Generated by Claude Code

- ScopedCssBundleImports.targets: capture the opening quote in the
  @import rewrite so double-quoted imports keep balanced quotes
  instead of producing malformed CSS; also match whitespace-free
  @import'...' forms.
- ConnectServerConfiguration.razor: add form-control to the five
  class-less inputs so they render as Bootstrap inputs instead of
  browser-default controls once Blazor stamps validation classes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants