Right edge toolbar#16
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| function buildToolbar() { | ||
| var current = document.documentElement.getAttribute('data-theme') || 'bold'; | ||
|
|
||
| var toolbar = document.createElement('div'); |
There was a problem hiding this comment.
Low – PREVIEW object duplicates CSS custom properties
These hex values are a second source of truth alongside --theme-*-accent and the per-theme variable blocks in shared.css. The comment on L25 acknowledges this, but it's easy to forget when tweaking a theme color.
Consider pulling the values from the computed style at runtime instead, so there's only one source to maintain:
function getThemeAccent(themeName) {
// Apply theme class temporarily to :root to read its CSS vars
var prev = document.documentElement.getAttribute('data-theme');
document.documentElement.setAttribute('data-theme', themeName);
var accent = getComputedStyle(document.documentElement)
.getPropertyValue('--accent').trim();
document.documentElement.setAttribute('data-theme', prev);
return accent;
}This is admittedly more complex and has a layout-thrash cost — acceptable if called lazily (only when the preview panel first needs a theme). For the current 5-theme hardcoded setup the static object is fine; just worth flagging before the theme count grows.
| preview.style.top = Math.max(4, br.top - tr.top + btn.offsetHeight / 2 - 40) + 'px'; | ||
| preview.classList.add('visible'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Low – innerHTML with string concatenation (safe today, worth documenting)
c values are hardcoded hex strings from the PREVIEW object above, so there's no injection risk in practice. However, the pattern of concatenating values into style="background:..." via innerHTML is fragile — if a chip value ever included a " or ; (e.g. a CSS expression), it could break the attribute boundary.
A safer equivalent that avoids innerHTML entirely:
var chipsContainer = preview.querySelector('.preview-chips');
chipsContainer.innerHTML = '';
p.chips.forEach(function(c) {
var chip = document.createElement('span');
chip.className = 'preview-chip';
chip.style.background = c; // style.background is safe — no HTML parsing
chipsContainer.appendChild(chip);
});element.style.background = value only applies the value as a CSS property; it can't inject markup.
| } | ||
|
|
||
| window.Toolbar = { syncActive: syncActive }; | ||
| })(); No newline at end of file |
There was a problem hiding this comment.
Nit – missing newline at end of file
All other JS files in the repo end with a newline. Add one here to stay consistent and avoid noisy diffs in future edits.
| 'use strict'; | ||
|
|
||
| // Desktop default is brutalist, mobile default is bold — default theme sits first in the bar. | ||
| var IS_MOBILE = window.matchMedia('(max-width: 768px)').matches; |
There was a problem hiding this comment.
Low – IS_MOBILE sniffed once at script load
matchMedia('(max-width: 768px)') is evaluated exactly once when the script runs. If a user resizes the window across the 768px breakpoint (or rotates a tablet from landscape to portrait), the THEMES array order won't update — the toolbar renders with whatever order was decided at load time.
This is unlikely to matter in practice (most users don't resize past that breakpoint mid-session), but worth a comment explaining the intentional choice:
// IS_MOBILE is intentionally evaluated once at load — the toolbar order
// is fixed for the session to avoid reordering under the user's cursor.
var IS_MOBILE = window.matchMedia('(max-width: 768px)').matches;Also note: this 768px breakpoint is now defined in three places — here, in the HTML <head> inline scripts, and in shared.css. If it ever needs changing, all three locations must be updated together.
Code ReviewThis PR cleanly replaces the old floating theme-switcher button and plane toggle with a consolidated right-edge toolbar. The refactor is well-scoped — the removal of the old cycling logic in What's good
IssuesAll issues are Low / Nit severity — nothing blocking:
See the inline comments for details and suggested fixes. Out-of-scope observations (pre-existing, not blocking this PR)
|
1 participant
No description provided.