fix(profiler): replace emoji with SVG icons in theme toggle - #175
Conversation
![clementine-oaklight[bot]](https://avatars.githubusercontent.com/in/3956685?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean swap from emoji to inline SVGs — straightforward.
Checked:
- Moon/sun SVGs are standard Feather-style icons ✅
stroke="currentColor"inherits button text color ✅- Toggle logic:
display:noneswap vstextContent— cleaner ✅ - Both flame and table HTML get identical icons ✅
- E501 ignore for
profiler.pymakes sense (inline HTML) ✅
Minor: manifest.json includes unrelated jsonschema 0.4.0 → 0.4.1 bump from make manifest picking up master changes. Non-blocking, just FYI.
LGTM 👍
![milo-oaklight[bot]](https://avatars.githubusercontent.com/in/3956677?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. SVG icons are well-formed (Feather-style stroke icons), toggle logic correctly shows sun in dark mode / moon in light mode. The duplicated inline SVG blocks in _render_flame_html and _render_table_html are fine for now—extracting to a constant would be a minor future cleanup but not blocking.
E501 ignore for profiler.py is reasonable given the inline HTML strings.
![elena-oaklight[bot]](https://avatars.githubusercontent.com/in/4082499?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix — emoji rendering is notoriously inconsistent across browsers/OS, SVGs are the right call.
Verified
| Area | ✓ |
|---|---|
SVG attributes (viewBox, stroke="currentColor", 16×16) |
Correct, inherits button text color |
| Toggle logic: moon visible in light mode → sun visible in dark mode | Matches original emoji behavior |
Initial state (display:none on sun) |
Correct — page starts light-ish |
| Both render paths (flame + table) updated consistently | ✓ |
| E501 ignore for inline HTML strings | Reasonable |
| manifest.json also shows jsonschema 0.4.0→0.4.1 | Expected from #173 merge + regenerate |
LGTM 👍
The dark/light mode toggle button used emoji characters (🌙/☀) which render inconsistently across platforms and don't match stroke-icon conventions. Replace with inline SVG moon/sun icons that toggle visibility on theme switch. Bump to 0.1.1.
7d261fe to
1e6143a
Compare
1 participant
Summary
display:noneon theme switch instead of swappingtextContentprofiler/profiler.pyto E501 ignore list (inline HTML strings)Test plan
pytest profiler/test_profiler_correctness.py— 128 passedmake lint— clean