-
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
Enhancement/8661 pax success banner #8734
Conversation
Build files for f2f696f have been deleted. |
Size Change: +3.37 kB (+0.23%) Total Size: 1.47 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.
Largely looks good, just a note about dismissing the notification for both CTAs really 🙂
event.preventDefault(); | ||
|
||
setTimeout( () => { | ||
const widgetClass = '.googlesitekit-widget--partnerAdsPAX'; | ||
|
||
global.scrollTo( { | ||
top: getContextScrollTop( widgetClass, breakpoint ), | ||
behavior: 'smooth', | ||
} ); | ||
}, 50 ); |
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 realise now the ACs are a bit unclear on this, but we should dismiss the notification when either CTA actions are used. I've updated the ACs to clarify this, and we should add a call to setNotification( undefined );
here.
@@ -61,6 +62,7 @@ | |||
|
|||
.mdc-button { | |||
margin: 0 auto; | |||
min-height: 32px; // Default min-height across the plugin is 40px, but in this instance it shouldn't be that big. |
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 comment doesn't explain why it should be smaller, just that it should be, but that's already evident by the style declaration 😄
The comment can be removed unless you need/want to explain the reasoning, if it's complicated 🙂
min-height: 32px; // Default min-height across the plugin is 40px, but in this instance it shouldn't be that big. | |
min-height: 32px; |
…le/site-kit-wp into enhancement/8661-pax-success-banner.
Thanks @tofumatt PR updated |
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.
The PR here looks good but there are a few merge conflicts that need resolving, then we can merge 🙂
@tofumatt Conflicts fixed. E2E nightly seems to be failing for some unrelated test regarding admin-bar |
Summary
Addresses issue:
Relevant technical choices
show me
button is not present. The PAX success notification only has stories, but it is not added to the VRT, as that part of the IB test section is crossed.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