Skip to content

[Obsidian review blockers] - Step 7: Remove CSS review violations - #2789

Closed
logancyang wants to merge 1 commit into
codex/issue-285-06-strict-sourcefrom
codex/issue-285-07-css-compliance
Closed

[Obsidian review blockers] - Step 7: Remove CSS review violations#2789
logancyang wants to merge 1 commit into
codex/issue-285-06-strict-sourcefrom
codex/issue-285-07-css-compliance

Conversation

@logancyang

Copy link
Copy Markdown
Owner

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

  • Scope generated Tailwind utilities to body, allowing normal utilities to outrank Obsidian's generic component styles without !important.
  • Remove component-level important modifiers and retain the one genuinely competing Miyo indent through explicit selector specificity.
  • Replace the config-dialog parent :has() rule with an explicit modal host marker.
  • Replace mobile navbar detection with navbar variables that intentionally have no fallback, so legacy mobile builds retain their original minimal spacing.
  • Remove unused table :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

  • This PR does not add the local review command or CI workflow; that is the final PR in the stack.
  • This PR does not repair the pre-existing component-gallery esbuild configuration.

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 format
  • changed-file ESLint with zero warnings
  • Obsidian Stylelint config against src/styles/tailwind.css with zero warnings
  • 13 focused suites / 119 tests passed
  • gallery CSS and package-script suites: 4 tests passed
  • npm run build
  • generated styles.css contains neither !important nor :has(
  • npm run gallery:build: story and CSS generation passed; the existing gallery esbuild step still fails on SVG loaders and browser-bundled Node modules

The repository-wide npm run lint still reports pre-existing test/tooling findings; the changed files are clean. The next PR introduces a source-scoped zero-warning review gate.

Risk

Risk Mitigation
Raising Tailwind selector specificity changes cascade order Utilities are scoped only by body, matching the precedence previously achieved with important modifiers; the production CSS build and focused component suites pass.
Legacy mobile builds gain navbar padding Navbar declarations require Obsidian's --navbar-height and --navbar-bottom-offset; without them the declaration is invalid and the legacy padding rule remains active.
Config dialogs lose full-bleed bands All three install-modal hosts add the explicit marker, and the shared shell test verifies it adds no padded wrapper.

Stack

@logancyang
logancyang force-pushed the codex/issue-285-06-strict-source branch from 10e9864 to 6fc01f3 Compare August 8, 2026 23:34
@logancyang
logancyang force-pushed the codex/issue-285-07-css-compliance branch from f19c375 to 90d8400 Compare August 8, 2026 23:34
@logancyang
logancyang marked this pull request as ready for review August 8, 2026 23:41

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@logancyang

Copy link
Copy Markdown
Owner Author

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:

  • remove the global Tailwind specificity change;
  • preserve exact existing visual values instead of redesigning with theme tokens;
  • replace relational selectors with explicit local host classes only where required;
  • keep behavior-equivalent syntax normalization needed by error-level checks;
  • leave warning-only CSS findings unresolved when removing them would risk UI behavior.

The revised branch will be verified on its own immediate base, without relying on a later PR to compensate for it.

@logancyang

Copy link
Copy Markdown
Owner Author

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.

@logancyang logancyang closed this Aug 9, 2026
@logancyang

Copy link
Copy Markdown
Owner Author

Conservative risk disposition

Risk: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant