-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/cele 27 Layout manager wrapper component #9
Conversation
vidhya-metacell
commented
May 28, 2024
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.
Thanks for the PR @vidhya-metacell ! All looks good to me. There is only a small issue while compiling the project, almost nothing, either some default value missing, or the parameters of a component to declare as optional and it should be perfect like this :)
@@ -5,7 +5,7 @@ import { useState } from "react"; | |||
|
|||
const { white, brand600, gray100 } = vars; | |||
|
|||
const CustomSwitch = () => { | |||
const CustomSwitch = ({width, height, thumbDimension, checkedPosition}) => { |
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.
As the signature of the component changed, the new parameters should be also added when it is used in this component (here).
I can see that the parameter are kind of optional, so to not modify the CustomListItem
component to add the parameters, another solution would be to define default values, or an interface stating that the fields are optional :)
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.
Hey @aranega , thanks for the suggestion. It makes sense. I have made the changes.
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.
Thanks for the fix!