-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Editor Welcome Tour Component #47779
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com D53351-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2 |
apps/editing-toolkit/editing-toolkit-plugin/wpcom-block-editor-nux/src/tour-content.js
Outdated
Show resolved
Hide resolved
Looking super good. Here's a few pixel changes:
|
Thanks for the feedback @dubielzyk I was able to address them all, let's make a build of it on monday and upload it to the ephemeral site for testing. I tried to test the welcome tour in safari but had a few problems:
I applied a bit of a simple fix for the janky image loading, I just put a fixed height on the image container so that the welcome tour doesn't jump around as images are loading, you might still look into preloading images if you think that's necessary as well. I had intended to put this behind a feature toggle but we don't actually have access to our calypso feature flags from the editing toolkit plugin, we should find a way to A/B test this, we'll have to look into hat actual A/B testing options we have access to from the ETK plugin... |
Maybe checkout what @deBhal and @andrewserong are doing on the premium patterns side of things. See pbmo2S-Ca-p2#comment-1369 and D53680-code |
This is great progress. Two more things:
|
I think I figured out why. See pbAok1-1Dh-p2#comment-3502 |
Let's get this in the ETK, even if we're not shipping. One way to do that would be to create a new directory for this work and copy the files over so we're not modifying the current NUX tour. Then update the load_wpcom_block_editor_nux() method to conditionally load the new tour. For example:
So, if Some discussion here: Let me know if you have any questions! |
4d4d0b9
to
e9cd0fd
Compare
Done. Corners look good |
…l/47779\#issuecomment-739650477 and set veritical min height on body
apps/editing-toolkit/editing-toolkit-plugin/full-site-editing-plugin.php
Show resolved
Hide resolved
apps/editing-toolkit/editing-toolkit-plugin/full-site-editing-plugin.php
Outdated
Show resolved
Hide resolved
26019d7
to
fdf0a4e
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.
This feature is looking so good @autumnfjeld!
I've left a comment about a duplicate class name warning in PHPCS because of the duplication of the NUX status controller. I can think of a couple of potential ways of dealing with this:
-
If you don't mind the duplication with this class, you could rename it. Or, find a place to keep the shared PHP files between this feature and
wpcom-block-editor-nux
so it only needs to be defined once (this would be better so that there aren't two attempts to register effectively the same API endpoint). Or, -
Move the Editor Welcome Tour feature to be part of the
wpcom-block-editor
directory, and share code, which means you could also remove the duplication of thedisable-core-nux.js
andpublic-path.js
files. The downside of this is that it mightn't be as neat to switch the feature and on off. Because we're enqueueing the one JS bundle, to handle switching features on and off we might need to use awp_localize_script
call to pass a boolean to the JS to flag that the feature is enabled. E.g.
wp_localize_script(
'wpcom-block-editor-nux-script',
'wpcomBlockEditorWelcomeTourActivated',
true, // the boolean value of whether or not the feature is enabled
);
Then, in the JS, you could detect whether or not the feature is enabled by checking window.wpcomBlockEditorWelcomeTourActivated
.
Personally I don't mind a bit of duplication at all while we're exploring a new feature, it can make it much easier to try things without having to worry too much about affecting an existing feature, but it can also be a bit tricky when there are collisions (like with the API endpoint). So I think it'd be perfectly valid to keep the two separate if you think it makes sense for the experiment!
...n/wpcom-block-editor-welcome-tour/class-wp-rest-wpcom-block-editor-nux-status-controller.php
Outdated
Show resolved
Hide resolved
@andrewserong Thanks for all the suggestions!! :) I think I'm ok with leaving the duplicate javascript files (public-path.js, store.js, etc) because as you said, the state of the NUX and Welcome Tour code is currently exploration. I fixed the PHP duplication however.
I assume you mean |
That sounds like a good trade-off to me @autumnfjeld, and nice idea getting |
@@ -75,7 +75,7 @@ public function enqueue_script_and_style() { | |||
* Register the WPCOM Block Editor NUX endpoints. | |||
*/ | |||
public function register_rest_api() { | |||
require_once __DIR__ . '/class-wp-rest-wpcom-block-editor-nux-status-controller.php'; | |||
require_once __DIR__ . '/../wpcom-block-editor-welcome-tour/class-wp-rest-wpcom-block-editor-nux-status-controller.php'; | |||
$controller = new WP_REST_WPCOM_Block_Editor_NUX_Status_Controller(); | |||
$controller->register_rest_route(); |
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 forgot to add: because the other usage of this controller also attempts to register the route, we might just want to test that this doesn’t throw an error anywhere. I think you can safely call to register the same route twice, but we might want to double check (check that it doesn’t throw errors and that the endpoint behaves as we’d expect). The function register_rest_route says that it returns false if it can’t register an endpoint so I suspect it’ll safely silently fail the second time, or just override the first call, which is also fine 🤞
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.
@andrewserong Thanks so much for pointing this out! Got me to think/learn a bit more in depth about some WP & php concepts.
register the same route twice
If I understand you correctly, the concern is that class WP_REST_WPCOM_Block_Editor_NUX_Status_Controller
will be registered twice, once when NUX code loads and again when Tour code loads. However, only one of these will load at a time, it is an either/or situation for A/B testing. This code in full-site-editing-plugin.php decides which to load.
In my sandbox I tested both scenarios and I didn't observe any errors in my /tmp/php-errors
file or in dev tools. (Note: Adding define( 'SHOW_WELCOME_TOUR', true );
to 0-sandbox.php file will force the new Tour to show).
Does that address the issue? Or is there anything I'm missing/not understanding?
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.
@autumnfjeld perfect, that sounds good to me! And from reading the docs for register_rest_route
, it looks like even if it wasn't guarded behind that either/or condition we'd probably still be safe 👍
…to block append of portalParent instead of blocking the render in the subsequent WelcomeTourFrame
…to allow coexistence of NUX modal and WelcomeTour for A/B testing. After this, will reinstate the wpcom-block-editor-nux folder
…WelcomeTour needs to be wired up
…nt loader files in tour folder
…l/47779\#issuecomment-739650477 and set veritical min height on body
…w in editor and to return is_nux_enabled as true so Tour always shows
…ephemeral site call for testing
…ep the edited one in wpcom-block-editor-welcome-tour/
…ilable when WelcomeTourCard is rendered
1370735
to
72f0535
Compare
This PR continues from initial spike.
Project issue details user facing requirements and project links.
Changes proposed in this Pull Request
Details:
is_nux_enabled
or if we need to create another flag a different flag for tracking when editor is opened for the first timeremove old NUX modal code UNLESS we are doing an A/B testIssues:
<LaunchWpcomNuxTour />
component is unmounting. I did not observe the useEffect cleanup happening.Testing instructions
There are three places used to determine whether to show the NUX dialog, in the following order:
wpcom-block-editor-preferences['nux-status']
wpcom_block_editor_nux_status
1) reset local storage
You must clear local storage for the site you are working with (from the Application tab of the chrome dev tools)
2) Reset the user attribute
wpcom-block-editor-preferences['nux-status']
The user attribute 'wpcom-block-editor-preferences' => 'nux-status' is used to determine whether to show the welcome tour.
This flag is also cached in local storage in the browser
It can be reset like so from a sandbox: (for your user_id)
At this point reloading the page should show the tour again
3) Reset the user metadata field
wpcom_block_editor_nux_status
This field is only used if the user attribute hasn't been set yet
Fixes #47691