-
Notifications
You must be signed in to change notification settings - Fork 286
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
Ads new badge. #8453
Ads new badge. #8453
Conversation
Build files for d8c8881 have been deleted. |
Size Change: +270 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
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.
Thank you for your work here, @zutigrm!
What do you think about re-using the already existing NewBadge
component here? I understand that we may need to extend it a little to make the <Tooltip>
optional (i.e. wrap it with a <Tooltip>
only if tooltipTitle
is set), but it already has the necessary styling matching the Figma designs.
Let me know what you think, thank you!
I have prepared a quick POC describing what I'm proposing here, thanks!
@nfmohit Thanks for the suggestion. I guess it would make removal in one of the next sprints easier, as only component would be removed vs tracking few styles to remove as well. I will implement it with |
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.
Thank you @zutigrm. This looks much better now. This is back with you with a few additional comments, 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.
Thank you for addressing my feedback, @zutigrm! This looks complete, I just have two too minor comments, so minor that I've gone ahead and applied them myself. I hope that is okay with you :D
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.
LGTM, thank you @zutigrm !
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist