Fixed: Scope injected CSS to the viewer - #567
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change renames viewer CSS classes and selectors with the ChangesCSS namespace migration
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/minify-css.mjs`:
- Around line 28-34: Update scopeSelector so a universal selector is expanded to
match both the scope element and its descendants, preserving the existing
descendant-scoping behavior for other selectors and the root pseudo-class
handling.
- Around line 37-40: Update the minification visitor configuration in transform
to scope the diva-cube-grid keyframe name using the same Diva class scope as
scopeSelector. Preserve existing selector scoping while ensuring .throbber-cube
references the scoped animation and cannot collide with host stylesheet
keyframes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4ead643-70e2-4a31-b431-cd90f2cb1164
📒 Files selected for processing (2)
scripts/minify-css.mjssrc/View.elm
|
Did you try just changing the selectors of the specific CSS rules, instead of coding in an override? |
|
Yes, that's the alternative way. The issue is that we need to rename all the generic names ( Happy to go either way. If you prefer the rename approach, I can switch to that. |
|
Yes, I would prefer that. |
The injected stylesheet used generic names like .modal, .status, .thumbs,
and .canvas-label, plus `* { box-sizing }` and `:root { color-scheme }`, which
collided with host-app styles when Diva is embedded. Prefix the generic names
with `diva-`, and anchor the two element rules to `.diva-app`. Closes DDMAL#566.
1aa15fc to
a907f01
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser-tests/auth.spec.ts`:
- Line 471: Update the close-action locator in the auth test to use the renamed
diva-modal-close-action selector rendered by PageViewModal, replacing the
obsolete .modal-close-action button target while preserving the existing click
and hidden-state assertions.
In `@src/styles/modal.css`:
- Around line 63-68: Rename the remaining canvas-toolbar class family to the
diva-modal-prefixed equivalent across the toolbar implementation. Update emitted
class names in Toolbar.elm, definitions in toolbar.css, and the close-action
selectors in modal.css together, including both canvas-toolbar-button and
canvas-toolbar-item, so no unprefixed toolbar classes remain.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 879cdb26-a707-486c-a0be-5d44273a59d4
📒 Files selected for processing (12)
browser-tests/auth.spec.tssrc/View.elmsrc/View/CollectionExplorer.elmsrc/View/ManifestInfoModal.elmsrc/View/PageViewModal.elmsrc/View/Sidebar.elmsrc/View/Toolbar.elmsrc/styles/app.csssrc/styles/modal.csssrc/styles/sidebar.csssrc/styles/toolbar.csssrc/viewer-element.ts
| .diva-modal-actions { | ||
| display: flex; | ||
| gap: 8px; | ||
| } | ||
|
|
||
| .modal-close-action .canvas-toolbar-button { | ||
| .diva-modal-close-action .canvas-toolbar-button { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Rename the remaining canvas-toolbar-* classes.
The modal selectors are prefixed, but the close-action rules still target .canvas-toolbar-button and .canvas-toolbar-item. src/View/Toolbar.elm and src/styles/toolbar.css still emit and define these unprefixed classes. They remain global and can collide with host-page styles. Rename the full toolbar class family and update all producers and selectors together.
Proposed selector direction
- .diva-modal-close-action .canvas-toolbar-button
+ .diva-modal-close-action .diva-canvas-toolbar-button
- .diva-modal-close-action .canvas-toolbar-item
+ .diva-modal-close-action .diva-canvas-toolbar-itemAlso applies to: 80-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/styles/modal.css` around lines 63 - 68, Rename the remaining
canvas-toolbar class family to the diva-modal-prefixed equivalent across the
toolbar implementation. Update emitted class names in Toolbar.elm, definitions
in toolbar.css, and the close-action selectors in modal.css together, including
both canvas-toolbar-button and canvas-toolbar-item, so no unprefixed toolbar
classes remain.
|
CodeRabbit flagged that |
|
Let’s do everything. Fewer possibilities for conflicts in the future. |
|
I'll wait until you to the other prefixing before reviewing / testing. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/View/Sidebar.elm (1)
658-664: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftComplete the
diva-namespace migration for modifier classes.The reviewed files prefix the primary class names, but the CSS and Elm contract still uses generic
is-*modifiers. Host-page CSS can match these tokens on viewer elements. Rename each modifier in both the emitters and selectors before merging.
src/View/Sidebar.elm#L658-L664: Rename panel modifiers and apply the same change at Lines 33-36, 327-340, 471-475, 619-622, 734-737, 751-756, 803-807, and 844-847.src/View/Helpers.elm#L35-L39: Renameis-disabledandis-fullscreen.src/styles/app.css#L197-L209: Rename.is-hiddenand.is-fullscreen.src/styles/collection.css#L55-L66: Rename.is-activeand.is-scroll.src/styles/sidebar.css#L15-L21: Rename the remaining sidebar and contentsis-*selectors at Lines 41-44, 62-63, 81-85, 95-157, 167-171, and 231-257.src/styles/toolbar.css#L26-L29: Renameis-right,is-fullscreen, andis-disabled, including the related selectors at Lines 84-108 and 145-152.As per the PR objective, all CSS classes must use the
diva-prefix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/View/Sidebar.elm` around lines 658 - 664, Complete the diva- namespace migration by renaming every listed is-* modifier emitted by src/View/Sidebar.elm and src/View/Helpers.elm, and update all corresponding selectors in src/styles/app.css, src/styles/collection.css, src/styles/sidebar.css, and src/styles/toolbar.css, including the specified related ranges. Keep emitter and selector names consistent, ensuring every CSS class uses the diva- prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/View/Sidebar.elm`:
- Around line 658-664: Complete the diva- namespace migration by renaming every
listed is-* modifier emitted by src/View/Sidebar.elm and src/View/Helpers.elm,
and update all corresponding selectors in src/styles/app.css,
src/styles/collection.css, src/styles/sidebar.css, and src/styles/toolbar.css,
including the specified related ranges. Keep emitter and selector names
consistent, ensuring every CSS class uses the diva- prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d59b32e-a0a4-42f0-bdf6-e7090b5a6747
📒 Files selected for processing (11)
browser-tests/auth.spec.tsbrowser-tests/public-api.spec.tssrc/View/CollectionExplorer.elmsrc/View/Helpers.elmsrc/View/Sidebar.elmsrc/View/Toolbar.elmsrc/styles/app.csssrc/styles/collection.csssrc/styles/modal.csssrc/styles/sidebar.csssrc/styles/toolbar.css
🚧 Files skipped from review as they are similar to previous changes (4)
- browser-tests/auth.spec.ts
- src/View/CollectionExplorer.elm
- src/View/Toolbar.elm
- src/styles/modal.css
2 participants
Problem
Diva injects its stylesheet into the page at import time, and the rules aren't scoped. They apply to the whole document instead of just the viewer. That includes a
* { box-sizing }reset,:root { color-scheme }, and common class names like.modal,.modal-body,.status,.thumbs*, and.canvas-label. When Diva is embedded in another app, these collide with the host's own styles.See #566.
Fix
Two small changes:
diva-scopeclass to the viewer's root element (src/View.elm).scripts/minify-css.mjs). The build rewrites each selector so:rootbecomes.diva-scope, and everything else becomes a descendant of it. For example,.modalbecomes.diva-scope .modal.Summary by CodeRabbit
Style
diva-CSS namespace.Tests