-
Notifications
You must be signed in to change notification settings - Fork 1
Migrate Easy components #126
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: main
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.
I think its not looking too bad.
There are quite a few issues with TS.
I would take a look into the detailed css, once youre done 👍
Also we need to add the changelog using the changesets
| // PaginationLast | ||
| // ============================================================================ | ||
|
|
||
| export interface PaginationLastProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {} |
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.
Here are quite a few typescript errors still
| goToLast: () => void; | ||
| } | ||
|
|
||
| export function usePagination({ |
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.
I would probably put this to a separate file, and not export it from the component file itself
| import { cn } from "../../utilities"; | ||
|
|
||
| export const numberboxVariants = cva( | ||
| "flex h-10 w-10 items-center justify-center rounded-sm text-text-lg font-semibold", |
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.
I think the dimensions are not exactly the same as the ones we do have right now. Especially width and height.
| @@ -0,0 +1,106 @@ | |||
| import * as React from "react"; | |||
| import { cva, VariantProps } from "class-variance-authority"; | |||
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.
There are unused ts imports, variables, etc... in here.
Please also make sure there is some responsive behavior in here. Probably using a container query can make sense I guess. But at somepoint there needs ot be a wrap I feel like
| // EmptyStateActions - Container for action buttons | ||
| const emptyStateActionsVariants = cva("flex items-center justify-center gap-3"); | ||
|
|
||
| export interface EmptyStateActionsProps extends React.HTMLAttributes<HTMLDivElement> {} |
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.
again some ts errors.
Once you're done with it, I will need to check, so esp. spacing etc... match the design system
|
Before merging, we need to check #112 again, because a new radix dep gets added here |
Fixes Issue / Ticket
Description
✨ [Provide a brief description of the changes introduced by this pull request.]
Checklist
Please make sure that your pull request complies with the following requirements:
Code Quality:
Testing:
Documentation:
Review:
Security:
Screenshots (optional)
📸 [Include any relevant screenshots or visual representations of the changes.]
Additional Information
ℹ️ [Provide any additional information that might be useful for the reviewer.]