luci-theme-footstrap: add new theme - #8903
Conversation
|
I like it. Could be a spiritual successor of the OpenWrt 2020 theme. Ideally I'd like to apply the same improvements to the bootstrap default theme too, while keeping its current style. |
|
Thanks for a feedback @jow- !
Palettes are templated by design: adding one is a matter of filling in a set of variables, not touching the chrome. So a palette matching bootstrap's current look is a small amount of work.
That might cover the "same improvements, current style" case without backporting client-side navigation logic into bootstrap itself. Also theme can be much faster if this will be merged |
|
I love this @VizzleTF . 👍 Just out of curiosity, should/could we minify some files? The ones that are more generic (never changes). |
Thank you for feedback! I minified it in my repo, its saves around 15% (65kb instead of 75kb) i can do it here also, if it doesn’t need to be readable 😄 |
Ah okey. :) I'll let @jow- make that decision 👍 |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (3985204). The commit message matches the diff, so no commit-check notes.
Two functional findings (login-form parity with the base sysauth.ut, and a layout-default that is stated three times and disagrees once) plus four nits are inline. CI on 39852045 is green across all three FormalityCheck runs, so nothing here is CI-grounded.
Generated by Claude Code
3985204 to
4251198
Compare
…default once The theme-local sysauth.ut dropped auth_fields, auth_message, auth_html and auth_assets, which the dispatcher hands every sysauth template. With luci-plugin-2fa installed the OTP input was never drawn, so luci_otp was never posted and a router with 2FA enabled could not be logged into while this theme was active. The template exists only to pass blank_page: true and now stays a superset of the generic one; each conditional attribute carries a leading space because ucode emits none of its own. _resolvedDefault() still answered 'sidebar' after the built-in layout flipped to the top bar, so on a fresh install matchesSavedDefault() was false before the user touched anything and Reset-to-saved landed on the wrong layout. Three comments described an Appearance page at admin/system/appearance that the package never shipped, counted fourteen axes against AXIS_KEYS' twenty-one, and listed a po/ that luci.mk's Build/Prepare does not copy. Found by the review on openwrt/luci#8903; verified on all four owlab routers.
4251198 to
b7402d3
Compare
|
this theme is awesome, much much better than others like proton2025/aurora. |
|
The three more or less official themes are:
I'm inclined to take this but given that maintaining non-default themes is effort no one is willing to spend, one of the other themes likely has to be deprecated. I'd lean towards deprecating 2020 in favor to this but maybe let's hear the takes from @feckert @systemcrash @hnyman |
I'll be glad to keep maintaining the theme. If something changes in luci-base and it breaks, I also have a few other pet projects I've kept going for a couple of years now, like |
|
If a new theme is added, I would drop at least the 2020, but maybe also the legacy theme, which is likely not used by many. (Personally I use bootstrap, which works well) |
|
Been testing this since PR, and I am a fan. Clean, fast, simple and refreshing. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push 39852045 → b7402d3. The commit message is unchanged and still matches the diff, so no commit-check notes.
All six threads from the last round are addressed: sysauth.ut now carries auth_message / auth_fields / auth_html / auth_assets and is a proper superset of the generic template; _resolvedDefault().layout is top; the Makefile, header.ut and fs-prefs.js header comments and the menu-footstrap.js indentation are all corrected.
The new work in this revision is the third palette. The list is consistently extended across all five places that need it — cascade.css (light + dark blocks, same token set as the other palettes), head.ut's _sd_pal whitelist, head.ut's pre-paint, fs-prefs.js's new PALETTES array, and the fs-appearance.js select — and swapping enumAxis for the wallpaper/density validate-against-a-list shape is the right call for a three-valued axis.
One functional finding inline, in the factory that palette just moved out of: enumAxis derives its window.__fsSD field name as key.slice(3), which is wrong for its one remaining instance (fs-pattern-ink → pattern-ink, but the field is pattern_ink). Plus three nits. CI on b7402d3 is green across all three FormalityCheck runs, so nothing here is CI-grounded.
Generated by Claude Code
…rver sends enumAxis derived its window.__fsSD field name as the localStorage key minus 'fs-', giving 'pattern-ink' where head.ut emits 'pattern_ink'. sd() answered undefined for good, so the axis reported the built-in 'theme' whatever the router had saved: the page was pre-painted data-pattern-ink="original" while the Ink control read Theme, and pressing Save-as-default wrote 'theme' over the stored value. Reproduced and fixed on 25.12/apk and 24.10/opkg. colorAxis folds the hyphen too, where every key is one word today and the failure would be as silent tomorrow. Also corrected the comments that had stopped describing the code: the uci-defaults fall-through called the built-in layout "the sidebar default", the axis-shape inventory named a hueAxis that does not exist, omitted surfaceAxis, counted five instances against fifteen, and still listed updateCheck and a wallpaper that persists to the router; four CSS comments and the file tree in docs/css.md pointed at styles/01-fonts.css, deleted with the webfonts in 0.12.1. The sysauth whitespace rationale now states what ucode actually does. Found by the review on openwrt/luci#8903.
7273c92 to
98201ff
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push b7402d3 → 98201ff. The commit message is unchanged and still matches the diff, so no commit-check notes.
First, my ucode whitespace nit from the last round was wrong and your measurement is right — I checked the source rather than my recollection. lstrip_blocks/trim_blocks are not off by default the way I assumed: both the ucode CLI (main.c:531-532) and, more to the point, uhttpd's ucode plugin (ucode.c:197-201) set both to true, and loadfile() inherits the running VM's config and only overrides the keys it is handed (lib.c:5522) — so LuCI's loadfile(path, { raw_mode: false }) keeps them on. The lexer then strips \t\v\f\r before {% and the newline after %} with plain tags (lexer.c:992, lexer.c:1076), which reproduces your A\nBCD exactly. The leading spaces are load-bearing and the comment now states the rule correctly.
The enumAxis/colorAxis sdKey fold, the uci-defaults layout comment and the keep.d inventory are all correct against the tree — pattern_ink is what head.ut:119`` emits and what header.ut's `FS_AXES` reads back, and both `/etc/footstrap` uploads are listed with matching symlinks in the uci-defaults script.
One functional finding inline, in the sysauth.ut block this revision rewrote: the new unconditional auth_message precedence fixes the second dispatcher branch but breaks the first one, where a wrong password on a 2FA router now shows only the plugin's boilerplate. Plus two nits. CI on 98201ff is green across all three FormalityCheck runs, so nothing here is CI-grounded.
Generated by Claude Code
`auth_message` is not "the credentials were accepted". The dispatcher's first branch copies it out of get_challenges() BEFORE the password is checked (dispatcher.uc:956), and that call answers with a message on every pending result -- the literal 'Additional verification required' when the plugin has none of its own (authplugins.uc:312). So on a router running luci-plugin-2fa both it and `fuser` are set whenever a password is rejected, and the message-first spelling this theme took from luci-theme-bootstrap then rendered only the plugin's boilerplate: the user is never told the password was wrong. The generic template's `auth_message && !fuser` gets that case right and the other one wrong -- on the OTP step the credentials were correct. `auth_plugin` separates the two dispatcher branches: it is set only in the second (dispatcher.uc:1018), i.e. exactly when session_setup() accepted the credentials. The credentials alert is now gated on !auth_plugin and the backend's message renders beside it rather than instead of it, so all four states are distinct. The credentials alert also moves to .alert-message.error, the danger variant bootstrap uses here, because once both can appear together an amber box for each reads a rejected password as a routine notice. Measured on owrt2512 (25.12.4/apk) and owrt2410 (24.10.8/opkg): the template compiles under `ucode -T -c` on all four dev routers, the four scopes render through LuCI's own loadfile/render path with the expected alerts and no throw for the absent auth_plugin, and a real wrong-password POST returns 403 with .alert-message.error on both package managers. Patching the dispatcher's sysauth scope reproduces the bug on the shipped spelling: wrong password with a challenge pending rendered no credentials error at all. Also in this commit, all found by the same review round on openwrt/luci#8903: * tools/axes.mjs now holds the window.__fsSD field name every axis reads, running the deriving factories' own formula lifted out of fs-prefs.js rather than restating it -- a restated copy stays green while the JS says something else, which was measured. surfaceAxis joins the key scan too (21 -> 25). * three stale comments corrected: the "Appearance -> Background image" label, propAxis's worked example, and fs-sheets.js's page-ownership rationale, which named a LuCI file that exists on none of the four dev routers. Re-measured against what actually injects a sheet there.
98201ff to
717de96
Compare
717de96 to
247891c
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push 98201ff → 247891c.
All three threads from the last round are addressed and correct against the tree. sysauth.ut now renders the two alerts independently with the credentials one gated on !auth_plugin, which covers all four dispatcher states; the propAxis header comment matches the hyphen fold; and the keep.d / uci-defaults wording now uses the "Appearance -> Wallpaper -> File" spelling that fs-appearance.js actually builds.
The new work is a router-side webfont opt-in (font_sans / font_mono / fonts, the /etc/footstrap/fonts symlink and keep.d entry, a conditional fonts/fonts.css link and an unlayered <style>) plus one CSS narrowing. Two things I checked and found sound, since they are the parts most likely to be wrong: /etc/footstrap/fonts works as a directory in keep.d — sysupgrade's list_static_conffiles() passes the keep.d lines to find … \( -type f -o -type l \), which recurses — and the new <style data-fs-shell> cannot be mistaken for a view sheet or disturb the layer order, because VIEW_SHEETS exempts [data-fs-shell] on style as well as link and both new tags sit after the cascade.css anchor reassertLayerOrder() measures against. The .cbi-page-actions .cbi-dropdown:not(.btn):not(.cbi-button) narrowing also lines up with the real markup: luci.js:2197-2207 gives the Save & Apply combo cbi-dropdown btn cbi-button cbi-button-apply, so the reset no longer flattens it in the Apply unchecked state where .cbi-button-apply has dropped off.
One finding and two nits inline. CI on 247891c is green across all eight checks (three FormalityCheck runs, eslint, CodeQL, build / Test x86_64), so nothing here is CI-grounded.
Commit checks
- 247891c "luci-theme-footstrap: add new theme" — the message's font paragraph is now a description of only half of what the diff does. It says "The theme carries no webfonts: the font stacks name Manrope and JetBrains Mono first and the system stack after, so a machine with either installed uses it and one without falls through silently" — still true of the package contents, but this revision adds a router-side opt-in on top of that fallback: three new UCI options in
footstrap.settings(font_sans,font_mono,fonts) read byheader.ut/head.ut, a new/www/luci-static/footstrap/fontssymlink created by uci-defaults, a newkeep.dpath, and a conditional@font-facestylesheet link. A UCI contract and a new sysupgrade-preserved path are the kind of thing worth a sentence in the message, since the message is what a bisect or agit logsearch lands on. (The.cbi-page-actions .cbi-dropdownnarrowing is a detail, not worth a line.)
Generated by Claude Code
I feel exactly the same way. If a modern upstream gets merged I will use this instead of my current used theme https://github.com/jerrykuku/luci-theme-argon @VizzleTF Unless this is just a one-time "fire-and-forget" operation. Then I would say @jow- 👍 |
font_sans and font_mono are printed unescaped into one <style>, and the
charset whitelist has to admit both quote characters -- the shipped default
is `"Manrope",system-ui,...`. A quote does not END anything, it EXTENDS:
`font_sans=My"Font` opens a CSS string that runs to the end of the sheet and
swallows `; --fs-font-mono: ...; }` with it, so a router that set only the
mono font correctly lost it because of a typo in the other option.
Measured on owrt2512 with that value in place. Before: the element renders
`:root {--fs-font-sans: My"Font; --fs-font-mono: JetBrains Mono; }` as one
open string and neither property survives. After: the malformed value is
refused and `--fs-font-mono: JetBrains Mono` is printed alone, while
`"Manrope",system-ui` still renders in full. head.ut compiles under
`ucode -T -c` on all four dev routers and the behaviour was checked on both
package managers.
Also in this commit, from the same review round on openwrt/luci#8903:
* the webfont comments say what the package does -- read the three uci
options, serve /etc/footstrap/fonts, keep it across sysupgrade -- and where
the one-line installer lives, since it is in this repository and not in the
package: it names third-party font hosts, which the theme must not carry.
The manual path is spelled out beside it.
* fs-sheets.js's ownership example is corrected against the in-tree app: only
HexEditor's injector runs at module eval, the view's own sheet comes from
render(), and what makes either invasive is the bare selectors, not the
ones pinned to #file-manager-container.
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push 9b5930ed → dca65066. The commit message is byte-identical and still matches the diff, so no commit-check notes. The delta is one file: labelCells() trades the table-level "ask the last row" early-out for a per-row first-cell skip.
That is the right correction — the last-row probe was the thing that stalled on a captioned <tfoot> while bare rows kept arriving in the <tbody> above it, and asking per row removes the guess entirely rather than special-casing the shape that exposed it. Cost goes from O(1) to one attribute read per row on a fully-captioned table, which is the honest price for not having a wrong answer.
One finding inline, on the paragraph rather than the code: the new comment claims a property the first-cell test does not have. CI on dca65066 is green across all three FormalityCheck runs, so nothing here is CI-grounded.
Generated by Claude Code
901055f to
398426e
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push dca65066 → 398426e9. Same base (1c70b2b), one commit, message byte-identical and still matching the diff, so no commit-check notes. The delta is 408 added / 264 removed across seven files, and it is a genuine architectural simplification rather than another patch: overflow-wrap: break-word on a data table's cells restores an honest min-content floor, which makes overflows() answer truthfully again, which lets idTower(), textLines() and the whole 75-line wordFloor() canvas reconstruction be deleted instead of corrected. Trading a hand-rolled UAX #14 for the engine's own line-breaking is the right direction, and the WPA2-PSK/CCMP case (144px claimed against a real 93px) is a fair account of why.
The remedy ladder checks out on the property it rests on — the guard at each rung is the second measurement, so rungs 2 and 3 introduce no threshold, and CRAMPED really is the only number left. The class hygiene around it is consistent: every mark the ladder can write comes off at the top of the pass before anything is measured (rule 1), breakWidestColumn() unwinds its own mark when the second overflows() still says no, and the room > 0 early-out restores what the element had rather than deciding against a width it does not have. markBreakColumn()'s _fsBreakCol short-circuit is safe because the reset always runs first, so a poll that swaps whole <tr>s cannot leave the mark behind on rows that no longer exist.
Widening the roots to #modal_overlay is well argued and the two hazards it creates are both handled — a closed dialog is excluded per pass via modal-overlay-active rather than by a selector fixed at load, and MutationObserver.observe() replacing options for the same node really is why the body-class watcher has to be a second observer (#view absent, body as content host, silent loss of the childList registration). CI on 398426e9 is green across all eight checks, so nothing here is CI-grounded.
Two findings and two nits inline. The first is the one worth a look before merge: the new sticky-header rule was written for #view but is not scoped to it, and the same commit made dialogs a table root.
Generated by Claude Code
398426e to
e87e2e8
Compare
A client navigation drops uci's config cache so that an app reading uci.load()'s
return value as an existence check is not told its config is missing. But
luci-base's network.js loads `network`, `wireless` and `luci` exactly once —
initNetworkState() fills its own _state and every later call returns
Promise.resolve(_state) without touching uci again — while still answering out of
that cache: getWifiDevices() is uci.sections('wireless', 'wifi-device'), and
view/network/switch reads uci.sections('network', 'switch') in its own render().
Dropping those packages therefore did not refresh them; it left every consumer
reading an empty config for the rest of the document. Measured on 24.10, one
navigation away from Interfaces: uci.state.values {}, getWifiDevices() 2 -> 0,
against 2 on a full load. That is Status -> Channel Analysis with no band tabs and
Network -> Switch with no VLAN sections, both correct again after F5 — reported in
review on openwrt/luci#8903.
The three packages are now refilled in the same pass and the incoming view waits
for them: a cached module resolves within a microtask and would otherwise be
constructed against the cache it was about to read. Everything else is still
dropped, and nothing is spent when network.js is not in the document.
e87e2e8 to
c547efc
Compare
|
Hello @castillofrancodamian Good catch, and my fault. I had made client navigation drop uci's config cache — a few apps read Fixed in main — those three packages are put back in the same pass and the incoming view waits for them (24.10, one navigation away from Interfaces: 2 wifi devices on a click, was 0; 2 on a full load). Should be in v0.12.7. I also covered this class with gates: every menu page is now opened both ways and compared (content, uci cache, device count, console errors), and the assumptions the theme makes about luci-base are checked against the luci-base the router actually runs. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push 398426e9 → c547efc1. Same base (1c70b2b), one commit, message byte-identical and still matching the diff, so no commit-check notes. The delta is 91 added / 31 removed across four files.
All four threads from the last round are addressed, and three of them exactly as asked: cascade.css now scopes the bar-height offset to #view and gives #modal_overlay the top:0 form; reach()/unreach() replace the value-based teardown with per-element _fsTab/_fsRole/_fsNamed ownership and skip role="group" on a real <table>; SCROLLABLE is scrollables(). The _barH move lands next to publishBarHeight rather than above its comment block, which is a different placement from the one suggested but matches what _geom at fs-chrome.js:154 does in the same file, so it is consistent as written.
The CSS rewrite checks out on the thing :where() puts at risk. Both rules drop to (0,6,0), and the later declarations they have to keep beating are lower — .table .tr.table-titles .th{…background:transparent} at line 508 is (0,4,0) and sits in the same @layer theme block, so the sticky background still wins. The #modal_overlay branch is deliberately unconditional on layout, which is right: the overlay is its own scrollport and the bar never overlaps it. .fs-xscroll's ID-carrying first-column rule at line 540 cannot collide, since that class is only ever written to .table:not(.fs-dt).
The refill in fs-router.js rests on a real asymmetry and picks the right three packages: initNetworkState() at network.js:353 latches _state and never loads again, while initFirewallState() at firewall.js:11 re-load()s on every call and needs nothing. window.L.network is a sound probe — require() writes the instance onto L's prototype only after the module has been evaluated — and filtering the list against names is what keeps a no-wifi router from fetching wireless that hasSystemFeature('wifi') never loaded.
One question inline, on what the new await costs rather than on what it fixes, plus two nits. CI on c547efc1 is green across all three FormalityCheck runs, so nothing here is CI-grounded.
Generated by Claude Code
…epeating itself Three notes from review on openwrt/luci#8903: - fs-router.js now says what awaiting the refill costs and why the free alternative was not taken. Measured on the stand over 12 alternating navigations between two warm views: 136 ms median without the wait, 159 ms with it, plus an unawaited uci.changes() that `uci-loaded` triggers. Leaving the three packages out of the unload costs nothing but breaks the pages people edit: network.flushCache() reloads its ubus half while its uci half is a no-op on a still-cached package, so Interfaces and Wireless would render fresh device state over stale config. - fs-select.js: the block at the fs-xscroll call site was a near-verbatim copy of reach()'s own header and had gone stale on the one line that changed (a role is no longer written unconditionally). It now carries only what the header does not — the group-not-region choice. - fs-select.js: "Reported in review." is a reference the tree cannot resolve. Comments only; no behaviour, no changelog entry.
3483b3a to
5e1cdbe
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push c547efc1 → 5e1cdbe1. Same base (1c70b2b), one commit, message byte-identical and still matching the diff, so no commit-check notes. The delta is 62 added / 15 removed across six files, and it is smaller than it looks: three of the six are comment-only.
The comment work is the good half. flushUciCache() now answers what the awaited refill costs (136 ms → 159 ms median on a warm two-view alternation) and, more usefully, why the free alternative was rejected: leaving `network`/`wireless`/`luci` out of the unload would let `network.flushCache()` reload its ubus half while `uci.load()` no-ops on a still-cached package, so Interfaces and Wireless would paint fresh device state over stale config — a state a full load never produces. That is the right reason to pay for the wait, and it is now on the page instead of in a PR thread. In fs-select.js the duplicated accessibility rationale is folded back into [`reach()`'s header](https://github.com/openwrt/luci/blob/5e1cdbe18e4ce3fbaa15e5c1dde20700faa75663/themes/luci-theme-footstrap/htdocs/luci-static/resources/fs-select.js#L581-L598),`` which is where it belongs — the call site keeps only the group-not-region note, and every fact the deleted paragraph carried (the WebKit bug, SC 2.1.1/4.1.2, the never-overwrite rule) is either in that header or visible in the four hasAttribute() guards themselves.
The other half is a new user-visible feature — the theme adds itself to the "Powered by" footer — and it arrives under a commit message that still describes the package without mentioning it, and with no supporting note in the PR body or the thread. The mechanics are sound: nameThemeInFooter() runs after footer.ut has been parsed (the L.require('menu-footstrap') script tag is below the footer), the .fs-footer-self probe makes a second call a no-op, and the footer really does survive a client navigation since navigate() sweeps .fs-content's children while the footer is its sibling inside .fs-main. links[1].after() also lands the fragment ahead of the lua_active notice as the comment claims, because that template branch emits bare text rather than a third anchor. The question is not whether it works but whether it should be there at all, which is the one finding worth a decision before merge — see inline.
One finding, three nits inline. CI on 5e1cdbe1 is green across all eight checks, so nothing here is CI-grounded.
Generated by Claude Code
The gate demanded the installed version EQUAL the newest the feed serves, and CI is exactly the case where it cannot be: the live job installs the build under test, which the feed has not published yet — 0.12.8 on the router against 0.12.7 in the feed — so a correct install failed the opkg leg. The apk leg passed only because `apk list` includes the installed version among its candidates, which is the kind of accident that makes a gate look right until the other manager disagrees. What the installer must never do is leave a router BEHIND the feed; being ahead of it is what a pre-release router looks like. Also the three review nits from openwrt/luci#8903: the footer link is written with quoted keys and an array child like every other E() call in fs-chrome.js; `rel` is `noreferrer` in both places that link to the project (it implies noopener wherever it is honoured, and it is what footer.ut's own anchors carry); fs-version.js's REPO_URL comment names both readers now that the footer is one. And the footer's leading stops ignoring the Density axis: `26px` fixed became `max(26px, calc(var(--fs-type) * 2))`. A bare ratio would have tracked the axis straight back under the 24px this rule exists to hold — Compact is 0.9, so `line-height: 2` is 23.4px there, and Compact is where the sentence wraps first.
5e1cdbe to
a449945
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
Force-push 5e1cdbe1 → a449945e. Same base (1c70b2b), one commit, message byte-identical, so no commit-check notes. The delta is 17 added / 10 removed across four files and is almost entirely the follow-up to yesterday's rel nit.
The rel collapse checks out on both counts. versionLink in fs-appearance.js:566 and nameThemeInFooter() in fs-chrome.js:306 now spell it the same way as footer.ut's two anchors,`` so the theme has one spelling of the rule instead of two — and it is the spelling the rest of the tree prefers: bare rel="noreferrer" outnumbers the `noopener`-carrying forms 18 to 11 across luci's non-footstrap JS/ucode. No behaviour is lost either; `noreferrer` implies `noopener` per spec, and `target="_blank"` is implicitly `noopener` in current engines regardless.
The same hunk quotes the E() attribute keys, which brings nameThemeInFooter() in line with every other E() call in fs-chrome.js (lines 27, 35, 36, 387, 388 all quote), and wraps the label in an array — E() accepts either, so that is cosmetic. The fs-version.js edit is comment-only and now correctly names both consumers of REPO_URL rather than claiming "used once".
The one substantive line is the footer's line-height, which goes from a flat 26px to max(26px,calc(var(--fs-type) * 2)). --fs-type resolves to a length (calc(13px * var(--fs-density-type)), or the round()ed variant), so the calc() is well-formed, and at density 1 it evaluates to exactly the old 26px. The max() therefore only ever raises the value — at --fs-density-type: 1.15 the footer gets ~30px leading, while the compact .9 density stays pinned at the 26px floor. That reads as a deliberate floor rather than an oversight, and it is the right direction now that the footer carries a third link that can wrap.
Still open from the previous round, not re-posted inline: the commit message and PR body describe the package without mentioning that the theme writes itself into the "Powered by" footer. That is the only user-visible behaviour in the package that a reader of the log would not expect from the subject line.
CI on a449945e: three FormalityCheck runs completed green, the rest not yet reported. Nothing failed, so nothing here is CI-grounded.
Generated by Claude Code
a449945 to
8036368
Compare
Yep. That is the practice for themes in the main OpenWrt LuCI repo. They are seen as originating from the repo itself.
I do not like that the footer contains theme name & a link to an external site. None of the other themes do that (as you noticed). I propose that if you want to keep a link of that kind, you move the link to the "footstrap options" page, where it would be visible for those that look more closely to the possibilities offered by the theme. It could be styled more like "feedback on the theme to be given here:" Note also that there could be PRs and commits from others directly authored to the LuCI repo (instead of yours), so the question where the theme gets developed in future may be more ambiguous than you think. |
Okay, i'll remove that. Not big deal.
Of course, i understand that. |
8036368 to
4868929
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push 5e1cdbe1 → 48689290. Same base (1c70b2b), one commit, message byte-identical and still matching the diff, so no commit-check notes. The delta is 586 added / 327 removed across ten files, and it is the largest structural change this PR has seen: the "Loading view…" spinner path is gone and the incoming view now renders into a hidden second #view that is swapped in when it is ready.
The staging design holds up on the two things it rests on. getElementById() returning the first match in tree order really is what both of LuCI's #view lookups use — View.__init__'s spinner and DOM.content(document.getElementById('view'), nodes) at the paint — so an in-flight chain writes into the stage and not into the live page, and stageView()'s insertBefore(wrapper, contentHost.firstChild) is what keeps that true. Keeping the live element's identity and moving nodes through dom.content() rather than swapping the element is the right call for the reason given: fs-fit and fs-appearance bind observers to the node that exists at chrome init. And visibility:hidden;height:0 over display:none is load-bearing — view.offsetWidth - 2 at load.js:31 would take a zero from a display:none stage; it also re-queries #view on every updateGraph() call rather than latching the node, so the post-commit reads land on the live element. Serializing renders in exchange for deleting armRenderGuard(), repairStaleRender() and currentViewClass() is a good trade: three mechanisms that only cleaned up afterwards, replaced by one that makes the damage impossible.
readonlyForSegs() going from OR to AND is correct, and for the reason the comment gives. check_acl_depends() at dispatcher.uc:312 is handed the single concatenated ctx.acls list and returns writable as soon as any one group grants write, and resolve_page() computes resolved.node.readonly = !perm at :1083 only when that list is non-empty — which is exactly gated > 0 && gated === locked, since apply_tree_acls() at :436 marks a node readonly precisely when its own list yields no write. Skipping nodes with no depends.acl matches ctx_append() pushing nothing for them.
The session gate reads luci-base accurately on both signals: the 403 + X-LuCI-Login-Required pair is what Request.addInterceptor reacts to at luci.js:2730, the rpc interceptor's (msg, req) argument order is the deliberately swapped one rpc.js documents, and the reasoning for excluding access: false is right — handleCallReply()'s expect loop keeps a same-typed false and resolves, so it never reaches upstream's .catch. Retiring the footer line, VERSION and isReal leaves no dangling readers anywhere in the theme or the templates.
Two findings and three nits inline. The first finding is the one worth a look before merge: a superseded popstate leaves its saved scroll offset parked in a module variable for the next navigation to spend. CI on 48689290 is green across all eight checks, so nothing here is CI-grounded.
Generated by Claude Code
54636ab to
903785c
Compare
|
@VizzleTF Wouldn't it be better if the arrow on the logout button pointed to the right? I find it more intuitive. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-push 48689290 → 903785c7. Same base (1c70b2b), one commit, message byte-identical and still matching the diff, so no commit-check notes. The delta is 717 added / 69 removed across eleven files, and it is one theme: nothing that reads layout may run while the reader scrolls.
The core of it holds up. Reading movement from the scroll position rather than from the event stream is the right call and the comment gives the right reason — iOS momentum carries the page long after the finger is gone and does not reliably emit scroll through it, so an event-plus-quiet-timer declares the reader still in the middle of a glide. Sampling one offset per frame costs nothing next to the getBoundingClientRect() it is protecting. The onResize width filter is likewise well argued: nothing any fitter asks is about height (roomFor()/overflows() compare a table against its column, fitChrome() asks whether the menu fits beside the brand), and the one apparent counter-example — a vertical scrollbar appearing — takes width from the content box, so the observer sees it as the width change it is. Using e.contentRect instead of re-measuring inside the callback is the detail that makes it free.
The slot cache in fitTables() attacks the actual cause rather than compensating for the symptom, and keying it on the section frame instead of the table's parent is what makes it work at all — a poll tick hands the container a freshly rendered subtree, so the immediate parent is new on every tick and every lookup would miss. Applying the slot's answer before the first measurement is the ordering that matters: the tall full-width intermediate only ever existed because an unmarked table was laid out before anything had been decided. Keeping rows out of shapeOf() and using a row-count drop as the one signal that a remedy may be liftable is a neat way out of the one-way-ratchet problem without paying for a strip-measure-restore per tick.
Splitting the five passes into five registrations, and moving armGate() into the module that writes .fs-fitted, are both the right shape — but see the fs-fit.js comment: the registration call is still the one uncaught run, so the "page with no tables at all" failure the split was made to close is still reachable through it.
Four findings and one nit inline. The two worth a look before merge are the background-tab interval pause, which leaves LuCI's own poll tick running twice, and the scroller memo, whose cache key does not cover what its predicate measures — on a desktop sidebar layout that can switch the whole mid-scroll guard off. CI on 903785c7: eslint and all four FormalityCheck runs green, the x86_64 build and CodeQL still in progress; nothing failed, so nothing here is CI-grounded.
Generated by Claude Code
A standalone theme for OpenWrt 24.10 and newer. It ships no CSS framework and depends on nothing but luci-base: the package is the server chrome (ucode templates), one stylesheet and a set of theme JS modules, while every page is still rendered by luci-base's own view JS. One chrome morphs between a collapsible sidebar and a top bar, chosen by the user and remembered client-side. Light and dark modes, a palette and a density axis are applied before the first paint, so a reload never flashes the previous colours. A menu search covers the rendered tree, and a client-side router swaps the view without a full document load, refusing to reuse a document an app's own stylesheet has invaded. Third-party luci-app-* packages are the constraint it is built around: the chrome is namespaced (fs-*, --fs-*, [data-fs-chrome]) and is defended by not matching an app's selectors, while .cbi-* and #view stay shared surfaces an app is entitled to win on. The theme's own settings page is drawn with stock .cbi-value rows, ui.Select and ui.RangeSlider. The stylesheet is generated from a layered source kept in the theme's own repository and committed here as one file, the way the other themes carry theirs. The theme carries no webfonts: the font stacks name Manrope and JetBrains Mono first and the system stack after, so a machine with either installed uses it and one without falls through silently. A router may name fonts of its own on top of that fallback, through three UCI options in footstrap.settings that the templates read back: font_sans and font_mono are font-family stacks printed into an unlayered <style>, and `fonts` is the md5 of an @font-face sheet the admin installs under /etc/footstrap/fonts, which uci-defaults links under /www and keep.d preserves across sysupgrade. The package installs no faces and fetches nothing at run time; it only serves what is already there. A web manifest and its app icons ship with the theme, so the interface can be installed to a phone's home screen. Both are static files — a theme may not register a dispatcher node — which fixes start_url at uhttpd's default /cgi-bin/luci/ and the splash colour at the default palette's; the manifest is served as .json because uhttpd's compiled-in MIME map has no entry for .webmanifest. The client router looks up every luci-base surface it calls before it wires anything, and on a miss logs which name is gone and stays off, leaving the plain server-dispatched interface rather than a half-working client one. Signed-off-by: Ivan Kvashonkin <vizzlef@gmail.com>
903785c to
0c9e205
Compare




Pull request details
Description
Add Footstrap, a standalone theme for OpenWrt 24.10 and newer. It depends on
luci-baseand nothing else and ships no CSS framework; pages are rendered by luci-base's own view JS..cbi-valuerows,ui.Selectandui.RangeSlider.Developed at https://github.com/VizzleTF/luci-theme-footstrap
Navigation is client-side: a click swaps the view rather than reloading the document, so a page opens without the full-load round trip. Measured against bootstrap on a Netcore N60 Pro running 25.12.2, five runs, medians:
Method and per-page results:
https://github.com/VizzleTF/luci-theme-footstrap/blob/main/docs/benchmark.md
Discussed on the forum since July:
https://forum.openwrt.org/t/footstrap-new-theme-for-luci-2x-faster-than-bootstrap/251930
Screenshot or video of changes (if applicable)
Desktop
Mobile
Maintainer (preferred)
@VizzleTF
Tested on
OpenWrt version: OpenWrt 25.12.4 and OpenWrt 24.10.8 (aarch64_generic containers), plus OpenWrt 25.12.2 (r32802-f505120278) on a Netcore N60 Pro
LuCI version: this branch, built with the x86_64 master and 24.10 SDKs
Web browser(s): Chromium 151, Firefox 145, Safari 26
And a single release is downloaded 200–240 times