-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Feat: create Common/BlogPostCard component #6037
Conversation
…dlier to read version
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Could you let me know if you are sure about this? According to Radix documentation, |
Hi, @ovflowd! Thank you for your comment. I checked the Radix UI docs before removing the Here's a video showing the result of removing delay-no-shift-but-flickering.movAfter reading your comment, I have come up with a better way of avoiding the layout shift. This way, we can keep the I will push the changes now. This is how it looks like: delay-ms-and-layout-fixed.mov |
92bbb4c
to
b2a6d83
Compare
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.
Great work and the component layout exactly matches! I have added some reviews to help 😄
b2a6d83
to
ce260ed
Compare
Thank you for the kind review! I have applied most of the changes you have suggested, and replied to all of them. I have also left a couple of doubts for @ovflowd to solve, as neither of them are clear from the designs. |
ce260ed
to
a6af08e
Compare
Hi @bmuenzenmeyer, I have just pushed the latest changes, allowing the |
Hey there, are there still open questions for me? |
4680156
to
fe1496e
Compare
Hi, @ovflowd. I have pushed the changes you have suggested, along with new unit tests for the |
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 know I've left a few last comments but after that I swear, this is good to go.
…ts to Common/BlogPostCard
fe1496e
to
040f26f
Compare
No worries! Thanks for your suggestions and for your feedback. I have just pushed the changes 🚀 |
Hey @danielmarcano just wanted to again appreciate the fantastic work you've been doing. We truly appreciate it! |
That's really kind of you, @ovflowd. Thank you for sharing these thoughts with me! |
Description
Common/Card
component, based on these Figma designs.Preview
andAvatarGroup
components, and semantic HTML tags to keep it as accessible as possible.The PR includes two minor fixes:
Common/AvatarGroup/Avatar
: Update component styles as it was creating a layout shift issue, which is not accessible, as it increases Cumulative Layout Shift, a Web Core Vital that is supposed to stay as low as possible. Here's a short video demonstrating this:delay-ms-issue.mov
Common/Preview
: theh1
tag has been replaced with anh2
one, so as to avoid having more than oneh1
tag per page, as it's not considered a good practice, and is also better for screen reader users.Validation
card-component.mov
Avatar
component:delay-ms-and-layout-fixed.mov
Common/BlogPostCard
componentRelated Issues
Closes #6033.
Check List
npx turbo lint
to ensure the code follows the style guide. And runnpx turbo lint:fix
to fix the style errors if necessary.npx turbo format
to ensure the code follows the style guide.npx turbo test
to check if all tests are passing.