Skip to content
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

Migrate block editor nux to createReduxStore #74454

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

noahtallen
Copy link
Contributor

@noahtallen noahtallen commented Mar 15, 2023

see #74399 for more info

Proposed Changes

Migrates the block editor nux (new user experience) ETK feature to createReduxStore

While there are other references to automattic/wpcom-welcome-guide in the codebase, they're in different webpack bundles, so I continued using the string-based store identifier approach.

Testing Instructions

TBD... you definitely start by syncing ETK. Create a new account and sandbox the new test site. At that point it might show up?

Added a couple people who recently touched these files for tips and maybe a review ;)

@noahtallen noahtallen requested a review from a team March 15, 2023 01:04
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 15, 2023
@github-actions
Copy link

github-actions bot commented Mar 15, 2023

@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit migrate-nux-to-crs on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@@ -153,16 +147,10 @@ function WelcomeTour( { siteIntent }: { siteIntent?: string } ) {
tourRating: {
enabled: true,
useTourRating: () => {
Copy link
Contributor Author

@noahtallen noahtallen Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this use of hooks is interesting 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They told us React was "just JavaScript", so why not hooks as config 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, fair enough! It stood out since I hadn't seen hooks used like this before :P

@matticbot
Copy link
Contributor

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.

Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a new account and sandbox the new test site. At that point it might show up?

You can also choose "Welcome Guide" from the kebab menu to make it reappear.

Everything else is working as expected

@@ -153,16 +147,10 @@ function WelcomeTour( { siteIntent }: { siteIntent?: string } ) {
tourRating: {
enabled: true,
useTourRating: () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They told us React was "just JavaScript", so why not hooks as config 😄

actions,
selectors,
controls,
persist: true, // TODO @noahtallen: persist plugin probably doesn't work here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do persist store data. This is it what the store looks like without the ETK patch.

CleanShot 2023-03-15 at 21 06 09@2x

And we do rely on it for caching so we're not always fetching the "should they see the welcome tour" flag from the server forever into the future.

And you're right it no longer appears to work with this patch applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate! I think the persist plugin may not have been migrated to work with the new store registration, so that may require more work before we can migrate this one.

Thanks for taking a look!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for context, the persistence plugin definitely isn't compatible with the more modern registration methods :( See WordPress/gutenberg#11449

Another question is whether we could use the editor preferences layer instead, which does support browser persistence

@noahtallen
Copy link
Contributor Author

Labeling as blocked until we get more info on what to do about the persistence plugin

@noahtallen noahtallen force-pushed the migrate-nux-to-crs branch from 4400ee2 to 5137bf3 Compare July 20, 2023 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Blocked / Hold [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants