-
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
An initial Version of my Component Library #1
base: main
Are you sure you want to change the base?
Conversation
I have reviewed your cmponent lib, everything is nicely done 👌
|
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.
Hi @hrshmistry,
Such a cool library you made. Loved it. The avatar and card section was quite attractive to the eye. Bohot e acchaa.
Few comments after going through:
- It is always a good idea to remove all commented out code. I have commented in one of the files but you should do it for all the files.
rem
makes thing responsive a bit. So, it is better to use that.px
can be used when no other options are available.
Again, nicely organised code and good naming conventions. Nothing very serious. Just some small stuffs :)
/* align-items: center; | ||
height: 700px; */ |
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.
It's always a good practice to remove commented-out code to avoid distraction to the reviewer.
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.
yes absolutely, I will remove the commented code and refactor it.
border-radius: 50%; | ||
width: 50px; | ||
height: 50px; | ||
margin: 0 0.5rem; |
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.
It's better to follow a particular sizing unit inside a class or inside an attribute. rem
makes things more responsive whereas px
doesn't.
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.
Sure thing it makes sense to use rem
most of the time. I will change accordingly.
@shubambhasin Glad, you liked it, I have also noticed that issue Now fixing it soon and yes thank you for reviewing 😄 |
Thank you @tuhindas30 😁, currently there are only a few components in the library but I will implement more thanks again for reviewing 🙌 |
An initial version of my Component Library,
Hello everyone I have done my initial version of Component Library, I tried to make it responsive to mobile yet things like the menu are yet to be added. I tried to follow best practices but I need more practice so your feedback is important!
please give your reviews and feedback, where can I improve it.
Deployed link of development branch: Compact CSS
Thanks.