-
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
Add Top Pages KMW. #9420
Add Top Pages KMW. #9420
Conversation
Build files for 0f7183b have been deleted. |
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.
Looks good, but let's remove the VRTs here since they are testing something quite generic so they don't add much.
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.
Let's remove these VRTs; they aren't adding much since these styles are already tested elsewhere, and our VRT suite is quite large.
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 removed all VRT's initially except for ready and view only ready states, as these are still specific to this widget which uses a bit different options than regular metric, and it will useful to have at least that state, so if something get's broken in the future it is easy to spot.
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.
What about this VRT is that specific? The visual output is very similar to https://google.github.io/site-kit-wp/storybook/develop/?path=/story/key-metrics-topcitieswidget--ready
I don't think they need to stay, if there's different logic to be tested maybe Jest tests would be better. 🤷🏻♂️
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.
You are correct, that's a valid point. Visually indeed it will not bring a value, and stories do cover the logic nuance needed in this widget already. I will remove the remaining scenarios
ReadyViewOnly.scenario = { | ||
label: 'KeyMetrics/TopPagesDrivingLeadsWidget/ReadyViewOnly', | ||
}; |
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 think we can remove all the VRTs in this file, as mentioned below 🙂
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.
Left the reply on the above comment
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.
Looks like there are some merge conflicts here, can you resolve them?
I'm fine with leaving the remaining VRTs in—I don't think they're that useful really but I don't feel too strongly about it 😅
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.
What about this VRT is that specific? The visual output is very similar to https://google.github.io/site-kit-wp/storybook/develop/?path=/story/key-metrics-topcitieswidget--ready
I don't think they need to stay, if there's different logic to be tested maybe Jest tests would be better. 🤷🏻♂️
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.
Oh, also, you should check if you want to add that new widget to the list added in #9418, eg. to keyMetricsGA4WidgetsNonACR
. I don't think this is an ACR widget, right? 🤔
Thanks @tofumatt PR updated, and merge conflicts resolved.
This is an ACR widget, we did change the approach in 9418 to list non ACR ones, instead of ACR widgets as initially though, for this purpose, of not including them every time they get merged. So in this case no changes will be needed |
Size Change: +633 B (+0.03%) Total Size: 1.85 MB
ℹ️ View Unchanged
|
Summary
Addresses issue:
Top pages driving leads
ACR KMW #9153Relevant 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