-
Notifications
You must be signed in to change notification settings - Fork 274
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
Add partial data badges #8727
Add partial data badges #8727
Conversation
Build files for 59d2419 have been deleted. |
Size Change: +19.9 kB (+1.38%) Total Size: 1.46 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.
Thanks @nfmohit, nice work here. Just making another case for separate components for mobile and desktop, considering their different appearance (Badge vs Notice). Let me know what you think.
...alytics-4/components/audience-segmentation/dashboard/AudienceTile/AudienceTilePagesMetric.js
Outdated
Show resolved
Hide resolved
...alytics-4/components/audience-segmentation/dashboard/AudienceTile/AudienceTilePagesMetric.js
Show resolved
Hide resolved
I agree, updated, 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.
Excellent work @nfmohit! Thanks for being patient with me and addressing my feedback. I've found one small tweak after the refactor, I'll push the fix myself and get this merged. Cheers!
export default function PartialDataBadge( { tooltipTitle } ) { | ||
return ( | ||
<span className="googlesitekit-audience-segmentation-partial-data-badge"> | ||
<Fragment> |
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.
This Fragment
is no longer needed after the refactor. I'll push a commit to remove this.
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.
Oops, looks like I mistakenly overlooked it, thank you for removing! 🦅
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 @nfmohit 🎉
Summary
Addresses issue:
Relevant technical choices
This PR adds the partial data badges in Audience Segmentation widget.
Deviation from IB
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