[Obsidian review blockers] - Step 7: Remove CSS review violations - #2789
[Obsidian review blockers] - Step 7: Remove CSS review violations#2789logancyang wants to merge 1 commit into
Conversation
10e9864 to
6fc01f3
Compare
f19c375 to
90d8400
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90d840099b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <div className="tw-flex tw-flex-col"> | ||
| <div className="tw-flex tw-flex-wrap tw-items-center tw-gap-2 tw-px-4 tw-pb-3 tw-pt-4"> | ||
| <h3 className="tw-m-0 tw-text-ui-medium tw-font-semibold tw-leading-tight tw-text-normal"> | ||
| {title} |
There was a problem hiding this comment.
Preserve the full-bleed config-dialog gallery stories
When a ConfigDialogShell story is opened in the component gallery, GalleryStoryModal creates an ordinary ReactModal without copilot-config-modal; removing the shell marker therefore leaves the new CSS selectors unmatched, so the gallery renders Obsidian's title and padding around the shell, producing a duplicate heading and inset dividers instead of the production state. Add an explicit, reusable way for this story host to opt into the config-modal chrome rather than silently weakening the adjacent story.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid for this abandoned CSS layer, but no longer applicable to the retained stack. PR #2789 was closed without merge; the current top stack at 3e257cb2 retains the copilot-config-dialog marker and the matching modal selectors, so production dialogs and gallery stories keep the full-bleed shell. I would not revive the warning-only CSS rewrite because avoiding that visual regression was the reason this layer was dropped.
|
Conservative-scope risk note before merge: The current CSS layer is too broad for a review-compliance-only issue. Global Tailwind body specificity, theme-token substitutions, and widespread utility changes can alter cascade, colors, stacking, mobile layout, and component appearance even if the intent is visual equivalence. I am revising this PR to address only the review blockers that can be fixed locally and safely:
The revised branch will be verified on its own immediate base, without relying on a later PR to compensate for it. |
|
Closing this stack layer under the revised conservative priority. Every finding addressed here is warning-level, while the proposed fixes alter global Tailwind specificity, component utility classes, colors, stacking, modal selectors, and mobile layout. That is disproportionate product risk for a review-only issue whose error-level findings can be cleared without changing UI. The CSS warnings are intentionally deferred. The pre-submission gate will continue to report them, but they will not block until each can be removed with visual-equivalence evidence. No code from this PR should be merged. |
Conservative risk dispositionRisk: High — deferred and closed. CSS warning cleanup can alter specificity, layout, interaction states, or theme compatibility even when the rules suggest mechanical replacements. The current CSS findings are warnings rather than approval-blocking errors. Recommendation: leave this PR closed and address CSS warnings separately with targeted visual coverage. The CI gate will continue to report them without blocking this issue. |
Why
Obsidian's review flagged
!important, relational:has()selectors, hard-coded colors, and other stylesheet patterns. This layer removes those blockers while preserving the existing UI precedence and mobile behavior.Closes part of #285.
What changed
body, allowing normal utilities to outrank Obsidian's generic component styles without!important.:has()rule with an explicit modal host marker.:has()variants and normalize the source stylesheet to Obsidian theme tokens and review-compatible CSS.Changed behavior
No intentional visual or interaction change. Existing compact inputs, modal edge-to-edge layout, component states, drag positioning, and mobile navbar clearance are retained through ordinary cascade specificity.
Non-goals
Screenshots
Not included. This is a stylesheet compliance pass intended to preserve rendering; scoped component tests, generated-CSS inspection, gallery CSS tests, Stylelint, and a production build provide the verification for this layer.
Verification
npm run formatsrc/styles/tailwind.csswith zero warningsnpm run buildstyles.csscontains neither!importantnor:has(npm run gallery:build: story and CSS generation passed; the existing gallery esbuild step still fails on SVG loaders and browser-bundled Node modulesThe repository-wide
npm run lintstill reports pre-existing test/tooling findings; the changed files are clean. The next PR introduces a source-scoped zero-warning review gate.Risk
body, matching the precedence previously achieved with important modifiers; the production CSS build and focused component suites pass.--navbar-heightand--navbar-bottom-offset; without them the declaration is invalid and the legacy padding rule remains active.Stack