task:1)Accodion and collapse 2)Model and offcanvas Done!. #97
Conversation
There was a problem hiding this comment.
Thank you for your work
However there are problems, I will list them progressively
There was a problem hiding this comment.
Overall, you did a great job, but there are some issues to address. Additionally, it's better to have a single change per pull request to make the review process easier. Having multiple changes can be frustrating during reviews. If possible, please fix all followings provided comments. If you encounter any complications or need further clarification, feel free to DM me on Discord.
| @@ -12,19 +11,19 @@ class Modal { | |||
| private state: string | |||
|
|
|||
| /** | |||
| * Modal Component | |||
| * Modal Component with built-in animations | |||
There was a problem hiding this comment.
The idea is not to apply the animations directly in place of the user, but to give the user the option of using build-in animations.
Maybe I haven't been very precise, but I'm gonna look at it again and give some good guidelines.
| @@ -36,22 +35,51 @@ class Modal { | |||
| const { showModal, hideModal, autoInitModal, isHidden } = initModal(modalElement, triggerButton, this.options); | |||
|
|
|||
| if (this.state === "open") { | |||
| showModal() | |||
| this.playOpenAnimation(); // Use animation when opening the modal | |||
There was a problem hiding this comment.
As I mentioned earlier, the animation shouldn’t be applied at this level. It’s better to give users the flexibility to handle it themselves. Right now, if you’ve noticed, to animate the modal, users must provide keyframes (either through options or attributes) (check here Instead of requiring users to create keyframes each time and then pass them in as animations, the idea is to offer ready-made animations. So, if users want to use pre-configured animations, they should have the option to easily enable them. Does that make things clearer?
There was a problem hiding this comment.
As I mentioned earlier, the animation shouldn’t be applied at this level. It’s better to give users the flexibility to handle it themselves. Right now, if you’ve noticed, to animate the modal, users must provide keyframes (either through options or attributes) (check here Instead of requiring users to create keyframes each time and then pass them in as animations, the idea is to offer ready-made animations. So, if users want to use pre-configured animations, they should have the option to easily enable them. Does that make things clearer?
Ah, I see what you mean! So, we would provide a set of pre-configured animations, possibly through an enum or schema for different animation types, and then users can choose which animation they want to activate. This way, they don’t need to define their own keyframes every time, but they still have the flexibility to select from our predefined options.
Or is it something different that you're referring to?
There was a problem hiding this comment.
@shreyannandanwar, will you be taking on the improvements, or would you like some help with it?
There was a problem hiding this comment.
Ah, I see what you mean! So, we would provide a set of pre-configured animations, possibly through an enum or schema for different animation types, and then users can choose which animation they want to activate. This way, they don’t need to define their own keyframes every time, but they still have the flexibility to select from our predefined options.
Hey @Adebesin-Cell you're right
| } | ||
|
|
||
| // Predefined open animation | ||
| private playOpenAnimation(): void { | ||
| this.modalElement.style.display = "block"; // Ensure it's visible before animation |
There was a problem hiding this comment.
We don’t handle this part; it's up to the user to decide how they want to manage the modal's appearance using data attributes. There's a data-state attribute that indicates whether the modal is visible or not, which users can control with CSS.
Take a look at the examples in the documentation to see what I mean.
| // Predefined open animation | ||
| private playOpenAnimation(): void { | ||
| this.modalElement.style.display = "block"; // Ensure it's visible before animation | ||
| this.modalElement.animate([ |
There was a problem hiding this comment.
This is exactly what we need, but instead of a single animation like this, we need a list of animations, each with a name. In the modal options, we should be able to specify which animation we want to apply to the modal content when opening or closing it. (in options or by using a specific data attribute)
| @@ -10,12 +10,13 @@ export type ModalContentAnimations = { | |||
| * Defines options for modal behavior. | |||
| */ | |||
| export type ModalOptions = { | |||
| defaultState?: "open" | "close", // Whether the modal should be open by default or not (default is close) | |||
| defaultState?: string, // Whether the modal should be open by default or not (default is close) | |||
There was a problem hiding this comment.
It’s important to keep the type as "open" | "close" here, as defaultState can only have these two values. It's essential to maintain this specific typing at this level.
| } | ||
|
|
||
| open() { | ||
| this.openOffCanvas() | ||
| // Predefined open animation for offcanvas |
There was a problem hiding this comment.
For the Offcanvas, it's the same as the modal, except that currently, there isn't an option to add keyframe animations like there is for the modal.
| } |
There was a problem hiding this comment.
Debounce and throttle aren’t really important at this level, as they could create a slight delay in execution. Here, we need the action to happen immediately. The only issue with the collapse/accordion is the state change on the trigger. Ideally, the trigger should change state at the same time as the content expands or collapses, but right now, the trigger only changes state at the end, which isn’t ideal.
|
sorry, won't be able to continue as my exam are going on |
Oh! Wishing you the best @shreyannandanwar 🔥 |
Tasks:
Accordion and Collapse:
Improve how the accordion and collapse components are triggered. Currently, the trigger behavior could be optimized to provide smoother and more reliable interactions.
Explore options for additional trigger types (e.g., hover, keyboard events) to improve accessibility and usability.
Modal and Offcanvas:
Add built-in animations for both modal and offcanvas components, integrating with the animate() method for quick, customizable animations.
The goal is to offer a range of predefined animations so users can easily apply them without needing to worry about implementing their own animation styles from scratch.
appropriate changes have been made