-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Sistent Card Component #6117
base: master
Are you sure you want to change the base?
Sistent Card Component #6117
Conversation
🚀 Preview for commit 33df114 at: https://6756a2a4e25791afc5b16274--layer5.netlify.app |
Signed-off-by: Anand-Theertha <[email protected]>
Signed-off-by: Anand-Theertha <[email protected]>
Signed-off-by: Anand-Theertha <[email protected]>
Signed-off-by: Anand-Theertha <[email protected]>
33df114
to
ade40a3
Compare
🚀 Preview for commit ade40a3 at: https://6756ad5a8a59a4c1a351345a--layer5.netlify.app |
Thank you, @Anand-Theertha 👍 |
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.
@Anand-Theertha I have reviewed your PR I have put some comments; you can look into it
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.
import { useStyledDarkMode } from "../../../../../theme/app/useStyledDarkMode"; | ||
|
||
const codes = [ | ||
`const cardOutlined = ( |
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.
Rather than creating a new component, you can directly pass as a child component to Card. So that I will be easier to copy for developers in the future.
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 made it this way to keep the code modular and abstract to make it more reusable. Do you still suggest I add this as a child component? @Vidit-Kushwaha
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.
Same increase in contrast between the text and background.
Will make these changes, thanks @Vidit-Kushwaha |
Hi @Anand-Theertha Facing issues merging your PR in Sistent? We are conducting a special tutorial on this Monday, December 16, 2024, at 7 AM Central (6:30 PM IST) in the website meeting. We'll cover:
Get more information: here 👥 Get involved:
Don’t miss out. 🚀 |
@Anand-Theertha did you made changes based on feedback yet? |
@Anand-Theertha checking in again? |
Will push these changes today, thanks for the reminder @sudhanshutech |
? |
Description
This PR fixes #5908. This PR adds a Card component to the Sistent components. Following are a few screenshots of how the implementation looks:
Notes for Reviewers
Signed commits