-
Notifications
You must be signed in to change notification settings - Fork 9
Chore: Migrate from radix-ui to base-ui #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the component library from Radix UI to Base UI, replacing four key components: Dialog, Dropdown, Toast, and Tooltip. The migration introduces breaking changes to the Toast API and updates component implementations to use Base UI's render prop pattern instead of Radix's asChild approach.
Key Changes:
- Replaced all Radix UI primitives with Base UI equivalents across four components
- Migrated Toast system to use Base UI's toast manager pattern with createToastManager() and useToastManager()
- Updated animation data attributes from data-state="open/closed" to data-open/data-closed throughout CSS
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/frappe-ui-react/package.json | Removed 4 @radix-ui packages and added @base-ui/react dependency |
| package-lock.json | Updated lockfile with base-ui dependencies and version bump to 1.0.2 |
| packages/frappe-ui-react/src/components/tooltip/tooltip.tsx | Migrated to Base UI Tooltip with render props and custom arrow positioning |
| packages/frappe-ui-react/src/components/tooltip/tooltip.stories.tsx | Added placement prop to story example |
| packages/frappe-ui-react/src/components/toast/types.ts | Restructured Toast types to work with Base UI's ToastObject and toast manager |
| packages/frappe-ui-react/src/components/toast/toastProvider.tsx | Implemented Base UI toast manager with createToastManager and useToastManager hooks |
| packages/frappe-ui-react/src/components/toast/toast.tsx | Refactored to use Base UI Toast primitives with new animation system |
| packages/frappe-ui-react/src/components/toast/toast.stories.tsx | Fixed whitespace formatting (tabs to spaces) |
| packages/frappe-ui-react/src/components/dropdown/dropdown.tsx | Migrated to Base UI Menu with render props and updated submenu structure |
| packages/frappe-ui-react/src/components/dialog/types.ts | Changed title type from ReactNode to ReactElement for stricter typing |
| packages/frappe-ui-react/src/components/dialog/dialog.tsx | Migrated to Base UI Dialog with Backdrop, Viewport, and Popup components |
| packages/frappe-ui-react/src/components/dialog/dialog.css | Updated animation selectors from data-state to data-open/data-closed attributes |
Co-authored-by: Aditya Dhade <[email protected]>
Co-authored-by: Aditya Dhade <[email protected]>
b1ink0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| return toastManager.add<ToastDataInternal>({ | ||
| id: options?.id || id, | ||
| timeout: durationInMs, | ||
| description: sanitizedMessage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be description: <span dangerouslySetInnerHTML={{ __html: sanitizedMessage }} /> ?
We are using DOMPurify.sanitize for sanitizedMessage, which means sanitizedMessage message supports HTML tags.
I think with the current implementation, this will render as:
This is <b>Bold</b> and this is <i>Italic</i> (The tags are visible as text) (existing issue)
for example if you try creating a test story with this component, you may be able see the issue.
export const HTMLUsage = () => {
const toast = useToasts();
const triggerToast = () => {
toast.info("This is <b>Bold</b> and this is <i>Italic</i>");
};
return (
<div className="flex flex-col gap-4 p-8">
<Button onClick={triggerToast}>Trigger HTML Toast</Button>
</div>
);
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right @mohdsayed. I have added the fix.
|
@abhishekxix Let's add test coverage for these components. |
Description
This PR migrates the component library from radix-ui to base-ui.
Components Migrated:
Dialog - Migrated from
@radix-ui/react-dialogto@base-ui/react/dialogDropdown - Migrated from
@radix-ui/react-dropdown-menuto@base-ui/react/menuToast - Migrated from
@radix-ui/react-toastto@base-ui/react/toastTooltip - Migrated from
@radix-ui/react-tooltipto@base-ui/react/tooltipRelevant Technical Choices
Key Changes:
Testing Instructions
Dialog
npm run storybookdisableOutsideClickToCloseis false)Dropdown
onClickhandlersToast
toast.promise()shows loading → success/error states correctlyclosable: false) don't show close button and don't auto-dismissTooltip
Additional Information:
Breaking Changes:
Screenshot/Screencast
N/A - This is a dependency migration with no visual changes expected. Components should look and behave identically to before.
Checklist
See #108