-
Notifications
You must be signed in to change notification settings - Fork 382
Prevent duplicated script to hide admin bar in onboarding wizard #6877
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
Prevent duplicated script to hide admin bar in onboarding wizard #6877
Conversation
…to avoid duplicate script render
c620b1a
to
c86439b
Compare
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 for your work, @NikhilJoshua!
The code looks good to me. I left one minor comment about unit tests.
add_action( 'wp_head', 'amp_remove_admin_bar_in_phone_preview' ); | ||
add_action( 'amp_post_template_head', 'amp_remove_admin_bar_in_phone_preview' ); |
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.
Should we maybe add a unit test to confirm that those two hooks are added?
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.
@delawski There's no test case for amp_init()
function so I didn't add it. Let me know your thoughts on 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.
I'll defer to @westonruter who's now free to do the final review.
Co-authored-by: Piotr Delawski <[email protected]>
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 E2E test is a nice touch 👍
// Checks for admin bar in iframe phone preview. | ||
const iframeElement = await page.$( 'iframe[name="amp-wizard-completion-preview"]' ); | ||
const previewFrame = await iframeElement.contentFrame(); | ||
await expect( previewFrame ).not.toMatchElement( '#wpadminbar' ); |
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.
Good!
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 :)
Co-authored-by: Piotr Delawski <[email protected]> Co-authored-by: Nikhil Joshua <[email protected]>
Summary
Fixes #6643
Changes in PR prevent scripts to be printed multiple times on the page to hide the admin bar in the onboarding wizard.
Checklist