[CHAIN] fix(ui): apply review fixes for the HeroUI to shadcn migration#11555
Open
alejandrobailo wants to merge 6 commits into
Open
Conversation
The dropdown separator dropped its dark variant when bg-default-200 was replaced with bg-zinc-200, and the role-form info icons relied on HeroUI's data-selected attribute that the shadcn Checkbox never sets. Use design tokens and group-has-[[data-state=checked]] instead, switch form.watch() reads to useWatch so the React Compiler tracks them, and restore the required indicator on the sign-up terms checkbox.
fonts.ts registers Fira Code under --font-mono but the theme pointed at the never-defined --font-geist-mono and no layout applied the variable. Also drop the dead .checkbox-update rule that referenced the removed HeroUI --background variable.
CopyButton was defined inside the component body, so every render created a new component type and React remounted the button, dropping focus and DOM state.
Sweep gray/slate/zinc literals missed by the token migration in Accordion, NavigationHeader, Toast, the legacy Skeleton and LinkCard, and remove the dead data-[hover]/data-[focus-visible] selectors on TableHead that only HeroUI's table ever set.
The rebuilt components had no render coverage: accordion controlled and uncontrolled selection, clipboard copy with timed feedback, and the password visibility toggle.
Update README tech list and AGENTS.md notes now that components/ui only holds cloud-overlay shims, remove the @heroui knip ignore, and fix a comment that still attributed the accordion scroll workaround to framer-motion.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
8 tasks
alejandrobailo
added
the
no-changelog
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
1 participant
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.
Context
Phase 7 of the HeroUI → shadcn/ui migration chain (#11532). A full review of the cumulative chain diff surfaced a few regressions and leftovers that are cheaper to fix on the chain tip than to distribute across the phase branches.
Description
Fixes
DropdownMenuSeparatorlost its dark-mode variant (bg-default-200 dark:bg-default-700→bg-zinc-200); now usesbg-border-neutral-secondary.data-selectedattribute, which the shadcnCheckboxnever sets; replaced withgroup-has-[[data-state=checked]]and the row marked asgroup.form.watch()during render, which the React Compiler cannot track (stale checked UI); switched touseWatch.isRequired); restored the asterisk.font-mononever resolved: the theme pointed at the undefined--font-geist-monoand no layout appliedfontMono.variable; both wired now (Fira Code).CodeSnippetdefined its copy button as a nested component, remounting the button on every render (caught by the new tests); inlined.Cleanup
gray/slate/zincliterals inAccordion,NavigationHeader,Toast, the legacySkeletonandLinkCard; removed deaddata-[hover]/data-[focus-visible]selectors onTableHeadand the dead.checkbox-updateCSS rule.AGENTS.mdnotes aboutcomponents/ui/, the@heroui/*knip ignore, and a comment still attributing the accordion scroll workaround to framer-motion.Tests
Accordion(controlled/uncontrolled, single/multiple, nested),CodeSnippet(clipboard write, formatter, copied-state feedback) andCustomInput(password visibility toggle, required indicator).Steps to review
pnpm install && pnpm run typecheck && pnpm vitest runinui/(1084 tests green).font-monotext (e.g. advanced mutelist editor) renders Fira Code.Checklist
UI
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.