Phase 1 + Phase 2: Drop external Bootstrap dependency, add opt-in Modern theme#373
Open
mike-thompson-day8 wants to merge 14 commits into
Open
Phase 1 + Phase 2: Drop external Bootstrap dependency, add opt-in Modern theme#373mike-thompson-day8 wants to merge 14 commits into
mike-thompson-day8 wants to merge 14 commits into
Conversation
Bundles in-progress Phase 2 modern theme work with follow-up component additions.
Pre-existing WIP (untracked/modified before this session):
- src/re_com/theme/modern.cljs: opt-in Modern theme (BS5-flavoured) — variables
layer plus `styles` multimethod for alert-box, button, checkbox, close-button,
dropdown, hyperlink, input-text, modal-panel, progress-bar, radio-button,
selection-list, tabs (horizontal/pill/bar)
- demo/re_demo/theme_picker.cljs: dropdown for live-switching :classic/:modern
- run/resources/public/assets/css/re-com.css: Bootstrap rules inlined (post-BS3
removal); .popover { left: 0 } moved here
- AGENTS.md, CLAUDE.md, README.md, CHANGELOG.md, package*.json: project docs and deps
Session additions (rc-vnu, rc-qmr, rc-88t, rc-50o, rc-ufh) — Modern theme
entries in modern.cljs for the remaining themable composites:
- datepicker: clean border + shadow-sm, primary-blue selected/today, muted
day labels, modern nav arrows
- daterange: clean panel borders, modern nav, typography
- slider: primary-blue accent
- input-time: modern field/icon container, hide-border state respected
- splits: modern splitter handle bar colors (border-strong / secondary on hover)
Verified visually in dev server with theme picker switched to Modern. All 64
tests pass.
Popover deferred (rc-4uu) pending parts/theme migration (rc-36h). Verified
popover.cljs:43 left:initial hack is still needed because re-com.css:6175
still defines `.popover { left: 0 }`.
Open follow-up: rc-w08 — investigate reported LHS nav width change when
toggling themes on Windows (could not reproduce from WSL Playwright).
Brings popover-title, popover-border, popover-content-wrapper, popover-anchor-wrapper, and popover-tooltip onto the modern part-structure / theme.cljs architecture used by alert-box. Theme methods preserve all legacy CSS classes so existing user styling and demo references keep working. Adds: - src/re_com/popover/theme.cljs with bootstrap/base methods per part - part-structure for each of the 5 popover components (using distinct :as-alias namespaces so auto-generated rc-* classes match legacy) - pre-theme/theme args via args/std + theme/args-desc Form-3 lifecycle behavior (refs, did-mount/did-update, position optimization atoms) is preserved; theme/comp is hoisted to the mount-time outer let so composition runs once per instance. Closes rc-36h. Unblocks rc-4uu (modern theme styling) and rc-0us (manual demo verification). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues found by playwright runtime validation of the migrated
popover components:
1. popover-content-wrapper validation rejected :title because the
part-structure declared ::pcw/title as a part name, colliding with
the :title top-level arg (the validator flagged it as
:part-top-level-unsupported). Renamed the internal sub-component
part to ::pcw/title-bar so users can still customize it via
:parts {:title-bar {...}} without conflicting with the :title arg.
2. popover-border and popover-content-wrapper crashed in
componentDidUpdate with "Cannot read properties of null
(reading 'getBoundingClientRect')" because debug/instrument's
->attr installs its own :ref function that only forwards to user
refs found in args[:attr][:ref]. The internal ref! callback was
overwritten and never fired, leaving the position/clipping atoms
nil. Moved (assoc-in [:attr :ref] ref!) AFTER debug/instrument so
the internal ref wins, matching the original component's
{:ref ref!} merged-last pattern.
Verified with playwright: popover-anchor-wrapper, popover-content-wrapper
(simple "click me" demo), and popover-tooltip all render correctly with
no page errors and no validation warnings.
Pre-existing bug filed as rc-gap: the "Complex Popover (dialog box)"
demo at /popovers shows a 💥 because popover-anchor-wrapper's else-
branch (introduced in commit 0ca6adb) breaks the [popover-body dialog-data on-change]
positional-args calling convention — confirmed reproduced on the original
popover.cljs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e-phase-2 # Conflicts: # CHANGELOG.md # src/re_com/popover.cljs
Adds modern visual styling to the regular popover (popover-border wrapper + content + popover-title), giving dialog/info popovers a softer BS5-flavoured surface: light-grey border, larger border-radius, gentle shadow, generous internal padding, and a subtle separator border below the title. popover-tooltip intentionally retains its classic dark-on-light look — the styles methods short-circuit when :tooltip-style? is true in the :re-com :state context. Tooltips are a distinct visual element and look intentionally different from regular popovers. Closes rc-4uu.
Three drift fixes against demo/re_demo/: 1. customization.cljs — Stale callout claimed "A complete implementation of our theme system will make bootstrap optional." Phase 1 already vendored Bootstrap into re-com.css, and Phase 2 added the opt-in modern theme. Updated to reflect both, with a cross-link to the /theme demo page. 2. popovers.cljs — popover-component-hierarchy table prose still said "popover-point" in two places where the keyword column was already renamed to :point in PR #372. Updated the descriptive text to match. 3. theme.cljs — Added a "Built-in modern theme" callout describing re-com.theme.modern, how to activate via reg-theme, and pointing users at the live theme picker dropdown in the title bar. Refined the "Note" to list the components that don't yet have modern styling (datepicker, daterange, input-time, slider, splits) instead of the vaguer "only some components support themes."
Phase 1 vendored Bootstrap rules into re-com.css. Existing migration notes covered the Bootstrap-stylesheet-vs-re-com-stylesheet ordering, but missed the consumer-overrides scenario: if a consumer's app loads their own stylesheet between bootstrap.css and re-com.css to override Bootstrap selectors, that stylesheet now needs to load after re-com.css (since re-com.css contains the Bootstrap rules) to win the cascade.
Phase 2 WIP checkpoint (7e9a7d2) accidentally swapped :justify :center for :padding "0 16px" on the demo's "re-com" title h-box, which left-aligned the title with 16px padding instead of centering it in the dark grey bar. Verified visually against the prod deploy (re-com.day8.com.au) — the revert restores the centered title.
The original entry was written before the rc-vnu/rc-qmr/rc-88t/ rc-50o/rc-ufh session additions in commit 7e9a7d2 (datepicker, daterange, slider, input-time, splits) and rc-4uu (popover) shipped. It claimed those components fell back to the classic look, which is no longer true — they all have modern styles in src/re_com/theme/ modern.cljs. Rewritten to: - list the actual covered components (now also includes datepicker, daterange, slider, input-time, h-split, v-split) - explain that layout + typography components are unaffected by design (no text re-flow) - call out the two intentional carve-outs: popover-tooltip retains the classic dark look, and Bootstrap-variant button classes are not restyled so colour-coded buttons still get their colour - drop the stale rc-5qt bead reference (that umbrella issue is closed; all per-component beads under it are also closed) title, p, label removed from the "covered" list — they don't have defmethods in modern.cljs, they just inherit classic typography.
Modern's ::rb/wrapper added `gap: 0.5rem` (8px) to the h-box wrapper. radio-button's classic theme already spaces its label via `:padding-left "8px"` on ::rb/label — so under Modern the two spacing mechanisms stacked, giving 16px instead of 8px (or, when the label is a plain string text-node, the gap created visible spacing where Classic had none, since text nodes don't accept `padding-left`). Either way the result was Modern radios visibly looser than Classic, contradicting the theme's "no text re-flow" promise. Drop the ::rb/wrapper override and rely on the existing classic mechanism in both themes. Checkbox is unaffected — it uses the gap mechanism in its classic theme (no padding on label) and Modern's ::cb/wrapper override correctly mirrors that. Verified visually with Playwright on /radio-button under both themes: identical rendering.
The "Note" callout on the /theme demo page claimed datepicker, daterange, input-time, slider, and splits don't have Modern styling and fall back to classic. Same staleness as the CHANGELOG entry I fixed earlier — those five components were styled in the rc-vnu/ rc-qmr/rc-88t/rc-50o/rc-ufh session adds, and popover landed in rc-4uu. Replaced with an accurate description: layout + typography components are unaffected by design; popover-tooltip and Bootstrap- variant buttons are intentional carve-outs; everything else themable has Modern styling.
The "Built-in modern theme" and "Note" callout boxes were placed at the top of the page next to the conceptual intro, which interrupted the flow into the worked examples. Moved to the bottom so the page reads: intro paragraphs → worked examples (themes, parts, layered, reactive) → operational notes (modern theme + coverage callouts) Conceptual material now flows uninterrupted into the demos.
@nextjournal/lang-clojure and codemirror were added in the Phase 2 WIP checkpoint (7e9a7d2), most likely while prototyping an embedded code editor for the demo. zero references in src/ or demo/ — the prototype was either abandoned or moved. They were also in the wrong section: under "dependencies" rather than "devDependencies", which means consumers installing re-com via npm pull them in too (CodeMirror 6 is not small). Removing both. Compile stays clean. 17 packages removed from node_modules (CodeMirror 6 + Clojure language mode + their transitive deps). package-lock shrinks by ~150 lines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is the merged outcome of the long-running
feature/modern-theme-phase-2branch — two phases of work shipping together for2.30.0:Phase 1 — Bootstrap as external dependency removed. Re-com no longer requires consumers to load
bootstrap.cssseparately. The Bootstrap 3.3.5 rules re-com depends on are vendored verbatim intore-com.css(preserved byte-for-byte against the upstream Bootstrap 3.3.5 source — verified). The classic re-com look is unchanged for any consumer who doesn't change theirindex.html.Phase 2 — Opt-in Modern theme. New
re-com.theme.modernnamespace ships a Bootstrap-5-flavoured visual theme. Activate from app code with(re-com.core/reg-theme re-com.theme.modern/theme). Typography is intentionally inherited from the classic look so apps adopting Modern incrementally don't see text re-flow.The popover modernization that PR #372 merged separately is also in this branch's history (via merge from master) but is not part of this PR's delta.
What changes
run/resources/public/assets/css/re-com.css(+6821 lines) — vendored Bootstrap 3.3.5 base rules (header-comment-delimited section). The original re-com rules are byte-identical to the prior version.src/re_com/theme/modern.cljs(+670 lines, new) — Modern theme withstylesmultimethod. Variables layer (BS5 palette, 4px spacing scale, BS5 radii, refined shadows, focus ring colours) plus per-componentdefmethod stylesfor: button, alert-box, progress-bar, input-text, checkbox, radio-button, horizontal/pill/bar tabs, dropdown, selection-list, modal-panel, popover (regular only), datepicker, daterange, slider, input-time, h-split, v-split, hyperlink, close-button.run/resources/public/index_dev.html/index_prod.html— removed the external<link href="bootstrap.css">.demo/re_demo/theme_picker.cljs(+33 lines, new) — title-bar dropdown for live Classic ↔ Modern switching using a wrapping theme function that derefs an atom inside the render path (reactive without remounting).demo/re_demo/utils.cljs— adds the theme-picker to the panel-title bar.demo/re_demo/core.cljs— small change to mount the picker.demo/re_demo/customization.cljs/theme.cljs/popovers.cljs— doc updates to reflect Phase 1 + Phase 2 reality (replaces stale "bootstrap will become optional" wording, adds Modern theme callouts, updates popover-component-hierarchy table for the renames from PR Add full :parts & :theme support to popover components #372).CHANGELOG.md—2.30.0 (Unreleased)entry covering Phase 1, Phase 2, and migration notes (including the CSS load-order edge case).package.json/package-lock.json— small demo-side dep tweaks.README.md— install instructions reflect that Bootstrap is no longer required.Modern theme component coverage
Has Modern styling: button, alert-box, progress-bar, input-text, checkbox, radio-button, horizontal-tabs, pill-tabs, bar-tabs, dropdown, selection-list, modal-panel, popover (regular), datepicker, daterange, slider, input-time, h-split, v-split, hyperlink, close-button.
Unaffected by Modern (deliberate):
h-box,v-box,box,gap,line,scroller,border) — pure layout, untouched.title,p,label) — typography is inherited from classic to avoid text re-flow.popover-tooltip— keeps its classic dark-on-light look (tooltips are a distinct visual element).btn-primary,btn-success,btn-info,btn-warning,btn-danger,btn-link) — preserved so colour-coded buttons still get their colour.Migration (also in CHANGELOG)
<link>.:class "btn-primary",:class "alert-info", etc.) — those classes still resolve because the rules are now inre-com.css.(tu/class props "btn")) — unchanged, the classes are still styled..btnor.popover), make sure it loads afterre-com.css. Previously you could rely onbootstrap.css→your-overrides.css→re-com.css, but now the Bootstrap rules live insidere-com.css, so user overrides need to come after it to win the cascade.Test plan
bb watchcompiles clean (0 warnings) — both:demoand:browser-testbuildsre-com.cssand diffed against the priorre-com.csscontent — byte-identical (only the new "vendored Bootstrap" section header differs)Out of scope
v-table/simple-v-table— these don't yet have parts/theme migration.