Skip to content

feat: card ui revamp #4630

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

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

feat: card ui revamp #4630

wants to merge 54 commits into from

Conversation

AmarTrebinjac
Copy link
Contributor

@AmarTrebinjac AmarTrebinjac commented Jun 30, 2025

Changes

Usually I'd post screenshots, but since this is about the post cards themselves, it's probably nicer to just use the preview! 😄

DESIGN

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Jira ticket

MI-915

Preview domain

https://mi-915.preview.app.daily.dev

Copy link

vercel bot commented Jun 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Jul 10, 2025 7:40am
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Jul 10, 2025 7:40am

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just blocking on the color, otherwise e2e looks good.

@@ -121,7 +121,7 @@ const colors = {
10: '#68A6FC',
20: '#5C9BFA',
30: '#508CF9',
40: '#427EF7',
40: '#5C9BFA',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should never change actually 🤯
Why is this different now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how i Interpreted @sshanzel comment here

@omBratteng
Copy link
Member

image
Probably should be some more space between these when hovering?

@omBratteng
Copy link
Member

More of a feedback to design, but I think there's way to little contrast between what is read and not read. Try to spot the read articles

image

@AmarTrebinjac
Copy link
Contributor Author

image Probably should be some more space between these when hovering?

the design specifically mentions no padding, although I guess we can add 1 px to make it look a little cleaner

@AmarTrebinjac
Copy link
Contributor Author

AmarTrebinjac commented Jul 9, 2025

More of a feedback to design, but I think there's way to little contrast between what is read and not read. Try to spot the read articles

image

It's a bit subtler indeed, but also wonder if it's due to you being used to the old style 😅

I'll raise it to design to see what they have to say!

RESPONSE

@AmarTrebinjac
Copy link
Contributor Author

AmarTrebinjac commented Jul 9, 2025

I can't seem to see the award button now. also there is still some shift now:

shift.mp4

@rebelchris
Clarified with design, and they are OK with this layout shift, so reverting back.

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :)

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing. This should be an experiment. My fault on this one 🙈 🙏

@@ -135,7 +138,7 @@ export const ArticleGrid = forwardRef(function ArticleGrid(
showFeedback && styles.read,
)}
>
<CardTextContainer>
<CardTextContainer className={classNames(postUiExp && 'mx-4')}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed we are primarily using mx-4 but the design seems to say 8px only which is mx-2.

Screenshot 2025-07-10 at 4 20 32 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sshanzel I think its a bit misleading because if you click in a level or two to see the actual text distance, its 16

Screen.Recording.2025-07-10.at.15.34.47.mov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the outer container does not havew the right padding, then yeah, mx-4 should be good.

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing. Just check whether the icon was accidentally removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants