fix(ui): resolve review findings across existing primitives - #843
Open
jim-fung wants to merge 1 commit into
Open
fix(ui): resolve review findings across existing primitives#843jim-fung wants to merge 1 commit into
jim-fung wants to merge 1 commit into
Conversation
Bugs: - sidebar: write the state cookie via document.cookie (Cookie Store API is unavailable in Firefox/Safari/non-secure contexts) and align the useMediaQuery md breakpoint with Tailwind's 768px so the 768-799px window no longer mixes desktop layout with mobile state - toggle-group: stop defaulting variant/size on the group so per-item props win; the group still renders its default look when unset - menu/context-menu: emit data-inset only when inset is set; the presence selector matched data-inset="false" and applied inset padding - combobox: merge triggerProps/clearProps className via cn instead of letting the spread drop the positioning classes - empty: EmptyMedia no longer spreads props on both the wrapper and the inner icon element (duplicate ids, double-fired handlers) - button: keep type="button" when render is a native button, and use aria-disabled (focusable, pointer-blocked) instead of an invalid disabled attribute for non-button renders - autocomplete: move popup visuals onto the Popup element so the enter/exit animations actually run, and add the missing exit states to Popover Consistency and conventions: - toolbar: honor orientation=vertical with flex-col - alert-dialog: drop the popup focus outline; Header/Footer support render like their dialog/sheet/drawer counterparts - accordion: apply consumer className to the panel element - radio-group: align items to the start like checkbox-group - group: fix the cn import path, default data-orientation, and flip GroupSeparator inside vertical groups - preview-card: use the preview-card-popup data-slot, expose side, add PreviewCardCreateHandle; add SheetCreateHandle - add missing data-slot attributes (checkbox-group, meter, scroll-area, field-control, field-validity) - remove dead utility classes in command, drawer, sheet, otp-field
|
@jim-fung is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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.
Summary
Fixes from an extensive review of the existing primitives (all 54 components, verified against the Base UI 1.6 typings and shipped docs,
apps/ui/AGENTS.mdconventions, and cross-sibling consistency).Bugs
document.cookie: the Cookie Store API is unavailable in Firefox/Safari and non-secure contexts, so every toggle threw an unhandledReferenceError. Also aligns theuseMediaQuerymdbreakpoint (800px) with Tailwind'smd(768px); between 768–799px the desktop layout rendered while the component behaved as mobile.variant/sizeon the group so per-item props are honored (<ToggleGroupItem variant="outline">previously rendered asdefault). Group-level visuals are unchanged when unset.data-insetonly when set:data-inset="false"still matches Tailwind v4's presence selectordata-inset:, which applied inset padding to non-inset items.triggerProps/clearPropsclassNameviacninstead of letting the spread drop the trigger/clear positioning classes.EmptyMediaspread props on both the wrapper and the inner icon element (duplicateids, double-fired handlers).type="button"whenrender={<button />}(implicit-submit risk in forms), and use focusablearia-disabled(styled, pointer-blocked) for non-button renders instead of an invaliddisabledattribute.<span>while Base UI applies the open/close state attributes to the Popup itself, so no enter/exit animation ran. Also adds the missing exit states to Popover.Consistency
Toolbarhonorsorientation="vertical"(was a hardcoded flex row while Base UI flipped the roving-tabindex keys).AlertDialogpopup drops its focus outline;AlertDialogHeader/AlertDialogFootersupportrenderlike their dialog/sheet/drawer counterparts.AccordionPanelapplies consumerclassNameto the panel element; missingdata-slots added (checkbox-group, meter, scroll-area, field-control, field-validity).RadioGroupmatchesCheckboxGroup'sitems-start;group.tsxfixes itscnimport path, always emitsdata-orientation, andGroupSeparatorflips horizontal inside vertical groups.PreviewCardPopupuses thepreview-card-popupdata-slot, exposesside;PreviewCardCreateHandleandSheetCreateHandleadded.Test plan
bun run typecheck, lint, formatregistry:validate-deps,registry:build,ui:syncNotes
button.tsxchanges overlap with refactor(ui): migrate Button to Base UI Button primitive #828 — happy to rebase and drop whatever that migration supersedes when it lands.