-
Notifications
You must be signed in to change notification settings - Fork 799
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
Block Theme Previews: show education modal when previewing theme #34935
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
63875a2
to
c9582fd
Compare
a1eb506
to
91aa2d7
Compare
17428af
to
4df6dec
Compare
@matt-west, I've sent you an invite to a test site |
Tagging @autumnfjeld to check the copy. |
0cef6c3
to
2824d13
Compare
2824d13
to
7400bb9
Compare
projects/packages/jetpack-mu-wpcom/src/features/block-theme-previews/modal.jsx
Show resolved
Hide resolved
projects/packages/jetpack-mu-wpcom/src/features/block-theme-previews/modal.jsx
Show resolved
Hide resolved
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.
@okmttdhr Thanks for testing!
I have this already: I somewhat remember that it worked but I'm not very sure now 🥲 Maybe it's not possible and the only way is to remove the old ETK modal when we are about to release 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.
Thanks @fushar!
I ran into the same issue with both modals being displayed at once. (Safari)
Super nit-pick. There’s a 1px line underneath the animation in the modal (Safari). Is there anything we can do about that?
The modal was correctly hidden on subsequent visits. 👍
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 testing, @matt-west!
Yes, looks like my "fix" does not work, so I'll just completely the old modal (via Automattic/wp-calypso#86385).
Nice catch! It was due to the computed height being a non-integer. I've fixed that, could you test again? Also, design-wise, is this good to go? |
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. Thanks @fushar!
64QjSf.mp4
Related to:
Proposed changes:
This PR shows an education modal when a user previews a block theme, for the first theme.
The modal can be dismissed and it won't be shown again, by storing a flag in a persisted redux store.
Infra changes
I added the following in
jetpack-mu-wpcom
package:[name]/[name].css
, similar to JS.jetpack-mu-wpcom
text domain for i18n.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
/themes
.Preview & Customize
.