-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(components): add dialog component #68
base: main
Are you sure you want to change the base?
Conversation
|
@sonnyt is attempting to deploy a commit to the Lemon Squeezy Team on Vercel. A member of the Team first needs to authorize it. |
Hey @sonnyt, Thank you for your contribution. It seems like you're headed in the right direction! Here are a few comments: You've used some Tailwind utilities that do not exist. These animation classes are available through the Wedges Tailwind CSS plugin:
We don't provide these: Also, Close button I'd use the Wedges Button component for the Dialog Close component to maintain consistency with the design. |
DialogContent.displayName = "DialogContent"; | ||
DialogOverlay.displayName = "DialogOverlay"; | ||
|
||
const Dialog = Object.assign(DialogPrimitive.Root, { |
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.
The idea behind Wedges components is to provide a versatile yet easy to use component for common use cases, equipped with the most useful props. Also, there is an option to enhance and personalize these components through the composition of additional components.
For instance, the Dialog
component can be used as follows:
<Dialog title="Title" isOpen={true}>
Content
</Dialog>
For more advanced use cases, the component can be composed with other components:
<Dialog.Root>
<Dialog.Title>Title</Dialog.Title>
<Dialog.Content>
Content
</Dialog.Content>
...
</Dialog.Root>
This flexibility allows for both easy implementation in straightforward scenarios and customizable complexity when needed.
```suggestion
const Dialog = Object.assign(DialogPrimitive.Root, {
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.
Perfect!
I will work on it next week :)
@brankoconjic I haven't forgotten about this PR. Been a little busy with job interviews. I will get back to it this week! 😊 |
|
||
export default function Example() { | ||
// You most likely don't need to use `containerRef` in your implementation. This is just for preview. | ||
const containerRef = useRef<HTMLDivElement>(null); |
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.
Mounting the dialog into a container rather than the document.body
so we can apply the dark and light themes during previews.
const transformDefaults = { | ||
translateX: "var(--tw-translate-x)", | ||
translateY: "var(--tw-translate-y)", | ||
rotate: "var(--tw-rotate)", | ||
skewX: "var(--tw-skew-x)", | ||
skewY: "var(--tw-skew-y)", | ||
scaleX: "var(--tw-scale-x)", | ||
scaleY: "var(--tw-scale-y)", | ||
}; | ||
|
||
const animateTransform = (transforms: Partial<typeof transformDefaults> = {}) => { | ||
return Object.entries({ ...transformDefaults, ...transforms }) | ||
.map(([key, value]) => `${key}(${value})`) | ||
.join(" "); | ||
}; |
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 noticed that currently, animations are overriding all transform functions. Ideally we only want to animate the functions we need and inherit the rest. This small utility function basically does that.
}, | ||
"100%": { | ||
opacity: "1", | ||
transform: "translateY(0px) scale(1)", | ||
transform: animateTransform({ translateY: "0", scaleX: "1", scaleY: "1" }), |
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.
So currently we are hardcoding the final state of the animations which basically is the initial state (translateY(0), scaleX(1)
).
I was thinking, what if we just remove them and fallback to default variables? Because currently animations override functions defined on the element classes.
For example:
<div className="translate-y-1/2 animate-wg-fade-in-up"></div>
The animation overrides the translate-y with 0
due to the final keyframe.
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.
fadeIn: { | ||
"0%": { | ||
opacity: "0", | ||
transform: animateTransform({ scaleX: ".97", scaleY: ".97" }), | ||
}, | ||
"100%": { | ||
opacity: "1", | ||
transform: animateTransform({ scaleX: "1", scaleY: "1" }), | ||
}, | ||
}, |
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.
Added basic fadeIn
animation
Any updates on this? @brankoconjic |
Sorry, I haven't had a chance to take a look into this yet. Still need to wait for the official Wedges design for this component before we can release it. I'm hoping to review the code next week. |
Started to add a dialog component. This is a still work in progress.
Am I heading the right direction @brankoconjic ? 😃