feat: replace prism with codemirror#378
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
usman-khalid
force-pushed
the
ae-improve
branch
2 times, most recently
from
2ee251c to
75ea0ff
Compare
|
Just a thought - if we're going down the route of introducing codemirror as a new vendor lib .. we may want to align https://tools.aem.live/tools/json2html-simulator/index.html to use the same too. WDYT? Right now it only uses Prism.js and code-editor is all custom built. |
shsteimer
left a comment
shsteimer
left a comment
There was a problem hiding this comment.
I don't like the idea of adding anew dependency just for this, juice doesn't seem worth it the squeeze to me. I could be convinced to change my mind there, but thats my initial POV.
if we can get rid of prism entirely as a result of this, then I'm much more open to it.
@sagarsane agreed - and there are two other places actually (ops/cache, and log-viewer also uses prism to display json responses for jobs in a modal) and then remove prism entirely as @shsteimer suggested. I think this is worth it given the feature set (particularly for admin edit and json2html which require editing json and markup extensively). It's a slightly larger library, but it's lazy loaded and we also get to remove a ton of plumbing (json2html simulator alone would cut around ~700 lines of code in the JS). Will push up a commit with all of the above and we can re-review. |
usman-khalid
changed the title
|
@claude review this PR. |
|
@claude re-review this PR. |
Code ReviewAll three issues from the previous review have been addressed:
PSI check is still redThe
The mobile 100 scores for admin-edit and json2html-simulator confirm the CodeMirror bundle is not blocking first paint (lazy-load in admin-edit is correct; the simulator's FCP/LCP are fine on mobile). The mobile 70s for cache-debugger and log-viewer look like pre-existing auth-gate timing (the cache tool calls The desktop "Timeout Exceeded" for admin-edit and json2html-simulator are worth investigating — Lighthouse couldn't complete scoring. This may be pre-existing or may be a one-time PSI infrastructure flake. Worth re-running PSI manually on the feature branch URLs to confirm before merging. No new issues introduced by this PRThe code is otherwise clean:
VerdictAPPROVE — pending confirmation that the desktop PSI timeouts are pre-existing (or a re-run showing them pass). The substantive correctness issue from the prior review is fixed. |
|
Replaced prism with codemirror entirely and have tested all the tools that were using it except the Ops cache debugger, if you can help with that @shsteimer |
shsteimer
left a comment
shsteimer
left a comment
There was a problem hiding this comment.
overall I think this looks good. I can;t find my ops cache debugger key right now. will do that after shutdown and we can merge
|
@shsteimer addressed your feedback. |
Conflict in tools/admin-edit/admin-edit.js: main (#348) refactored admin API access to go through a getAdminClient() factory in the new scripts/admin-compat.js (H5/H6 switch), and added a `loadPrismLibrary` import for the Prism-based syntax highlighter. Our branch had replaced that entire Prism code path with CodeMirror and dropped utils/prism. Resolution: - Take main's `getAdminClient` import; drop our old direct `import admin from '../../scripts/helix-admin.js'`. `admin` is now a `let` populated by `admin = await getAdminClient()` at init (which git auto-applied), so all `admin.raw` / `admin.status` / `admin.coordsFromURL` / `admin.suggestions` call sites keep working. - Drop `loadPrismLibrary` — no callers remain on this branch (the Prism paths in admin-edit are gone). Co-authored-by: Cursor <cursoragent@cursor.com>
…ight The prism-to-CodeMirror migration left the cache debugger's Purge Result modal and the log-viewer's admin-detail modal with a fixed height that always filled the viewport cap, leaving dead space for short responses. Switch to max-height so the viewer shrinks to fit and only scrolls when content exceeds the cap, matching the old <pre> behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The #editor host div had no tabindex, so keyboard users could never Tab into it to trigger the CodeMirror mount (a regression from the prism->codemirror switch in #378, where the original PR feedback removed the broken `focusin` trigger but didn't restore focusability). Switch to `focus` on a tabindex="0" host, guarded against the async mount stealing focus back if the user tabs away before it resolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…only viewers admin-edit and json2html-simulator already override .cm-gutters/.cm-activeLineGutter with light-dark() colors, but that styling was never added when prism was swapped for CodeMirror in the cache debugger and log-viewer, so their gutters fell back to CodeMirror's light-theme defaults in dark mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeMirror's default .cm-selectionBackground is a light-theme-only gray with no dark-mode awareness, making selected text nearly unreadable in the cache debugger and log-viewer read-only viewers. Override with light-dark() to match the gutter theming already added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd json2html-simulator Same fix as the cache debugger and log-viewer: CodeMirror's default .cm-selectionBackground is a light-theme-only color, unreadable in dark mode. Also switches all four tools from ad-hoc hex/rgba values to the project's --gray-*/--transparent-white-* tokens for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
indentWithTab rebinds Tab to indent, which traps keyboard focus inside editable CodeMirror instances (WCAG 2.1.2). Read-only viewers are excluded from tab order already (editable: false sets tabIndex -1) so they're unaffected. Add an Escape binding that blurs the editor for editable instances, plus a visible hint (wired via aria-describedby) telling users how to exit — the same convention GitHub's own CodeMirror-based editor uses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The pre-mount tabindex + once-only focus listener created a keyboard trap: after the first Escape-out, the outer wrapper div became a dead second tab stop with no listener left to hand focus into CodeMirror. CodeMirror now mounts only when Fetch is submitted, and its own contentEditable surface is naturally tabbable in place — no wrapper tabindex needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Replace prism with codemirror, which adds a few nice features:
Tools updated:
Test URLs:
Before:
After: