Repository navigation
fix: resolve the 3 critical findings from the code review - #7
Merged
Merged
Conversation
The CLI emitted component code whose imports it never installed: - Missing registryDependencies: button->spinner, modal->backdrop+iconbutton, drawer->backdrop, sidebar->tooltip, statscard->divider. - add only copied utils/cn.ts, but Modal/Drawer/ImageExpander also import utils/scrollLock. Now copies every non-test file under src/dashkit/utils in both add and init. - program.parse() -> parseAsync() so async action handlers are awaited. Adds a static registry test asserting every cross-component relative import is covered by registryDependencies, plus e2e CLI tests for compilable output.
- Sidebar discarded the setIsOpen returned by useSidebarState and passed a no-op, so calling setIsOpen from useSidebar() silently did nothing. Wire the real setter through and memoize the context value. - AccordionItem accepted a disabled prop but only painted a CSS class; the trigger still toggled on click. Plumb disabled through the item context and make AccordionTrigger set the disabled/aria-disabled attributes, guard the click handler, and expose aria-expanded. Adds tests for both behaviors.
- Modal & Drawer: role="dialog" + aria-modal, move focus into the overlay on open and restore it on close, and trap Tab within the dialog (shared utils/useFocusTrap). - Tabs: full ARIA (tablist/tab/tabpanel, aria-selected, aria-controls, roving tabindex) plus Arrow/Home/End keyboard navigation. - Dropdown & Popover triggers: promote plain-text triggers to an accessible, keyboard-operable button (role/tabIndex/aria-expanded) while leaving element-child triggers as plain wrappers to avoid nesting interactive semantics. - Tooltip: role="tooltip", aria-describedby association, show on focus, and clear the pending timer on unmount. - Toast: aria-live region with role=alert for errors / status otherwise. - IconButton: keep outline-none but add a focus-visible ring. Adds/updates component tests for each.
✅ Deploy Preview for dashkit ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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 fixes the three critical issues identified in the recent code review of the Dashkit UI project, organized as three independent commits.
1. CLI output did not compile (
fix(cli): make add output compile)registryDependencies:button→spinner,modal→backdrop+iconbutton,drawer→backdrop,sidebar→tooltip,statscard→divider.dashkit addpreviously emitted components referencing files it never installed.utils/incomplete:addonly copiedcn.ts, but Modal/Drawer/ImageExpander also importutils/scrollLock. Now every non-test file undersrc/dashkit/utils/is copied by bothaddandinit.program.parse()→parseAsync()so async action handlers are awaited.registryDependencies(it caught a real gap:badge→floatbadge), plus e2e CLI tests for compilable output.exec/cli.jsbundle.2. Functional bugs (
fix: make Sidebar.setIsOpen and Accordion.disabled actually work)Sidebardiscarded thesetIsOpenreturned byuseSidebarStateand passed a no-op; callingsetIsOpenfromuseSidebar()silently did nothing. Wired the real setter through and memoized the context.AccordionItemaccepteddisabledbut only painted a CSS class — the trigger still toggled.disablednow flows through the item context;AccordionTriggerblocks the click, setsdisabled/aria-disabled, and exposesaria-expanded.3. Accessibility (
fix(a11y): add dialog, tab, trigger, tooltip and toast semantics)role="dialog"+aria-modal, focus moves into the overlay on open and is restored on close, and Tab is trapped within the dialog (sharedutils/useFocusTrap).tablist/tab/tabpanel,aria-selected,aria-controls, roving tabindex) + Arrow/Home/End keyboard navigation.role/tabIndex/aria-expanded/aria-haspopup); element-child triggers stay plain wrappers to avoid nesting interactive semantics (keeps docs/tests valid).role="tooltip",aria-describedbyassociation, shows on focus, clears the pending timer on unmount.aria-liveregion withrole=alertfor errors /statusotherwise.outline-nonebut adds afocus-visiblering.Testing
bun run check:allpasses: 272 tests, lint clean,tsc+ build clean.Notes