-
-
Notifications
You must be signed in to change notification settings - Fork 335
Conversation
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 your contribution 🚀 In general it looks quite good, just some small comments:
The comments are always referring to the screenshot above them
- The text in lightmode isn't readable
- The badges aren't vertically centered (see badges next to them)
- On mobile they take up too much space, I think that we should move the badges into the collapsable section where currently the labels are shown, just on mobile though, what do you think?
Yeah sounds good will make the changes right away ! |
Hey @Cahllagerfeld I have made all the changes accordingly please review it and lmk if there is something that I should change further. Thank you ! |
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 @Cahllagerfeld I have made those changes do lmk if there's anything else to change ! |
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 @KlausMikhaelson 👋
I made a small adjustment to get rid of the custom breakpoints, as we can also use the tailwind ones + I extended the types so the typescript compiler is happy 🎉
We can get this merged now I think 🛹
Awesome thank you so much for the help ! @Cahllagerfeld |
Fixes Issue
Closes #204
Changes proposed
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers
I am willing to make any changes if they are required ! this is my first pr for eddiehub community so thank you !