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

Move and modularise guided tour state #43723

Merged
merged 6 commits into from Jul 30, 2020

Conversation

sgomes
Copy link
Contributor

@sgomes sgomes commented Jun 26, 2020

This PR is one of many working on modularising state in Calypso. This one handles guided tours, which until now live under ui but deserve their own top-level entry.

For more details on state modularisation, see the modularised state documentation and p4TIVU-9lM-p2.

Note: I added all the reviewers that GitHub suggested. Please feel free to ignore the review request or pull in someone else if you're not comfortable reviewing this PR. Thank you!

Changes proposed in this Pull Request

  • Move guided tours state out of ui and into a new top level entry
  • Modularise it
  • Split guided-tours/contexts monolithic module into smaller ones.

Testing instructions

Guided tour state is always loaded on (logged-in) boot, and that appears to be an intentional architectural decision. However, that makes it difficult to see the state going from unloaded to loaded, as it happens without user input on boot, so it's difficult to test the automatic state loading part of the PR. That's not a big concern, however; the important thing is to ensure that nothing is getting broken.

With that in mind, in order to verify that this change works correctly, please smoke test guided tour functionality and ensure that it continues to work as before.

@sgomes sgomes added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial Guided Tours State labels Jun 26, 2020
@sgomes sgomes requested review from lsinger, jsnajdr, scinos and a team June 26, 2020 12:12
@sgomes sgomes requested review from a team as code owners June 26, 2020 12:12
@matticbot
Copy link
Contributor

@sgomes sgomes added this to In progress in Modularized state migration via automation Jun 26, 2020
@matticbot
Copy link
Contributor

matticbot commented Jun 26, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~55 bytes removed 📉 [gzipped])

name        parsed_size           gzip_size
entry-main       -187 B  (-0.0%)      -55 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~896 bytes removed 📉 [gzipped])

name              parsed_size           gzip_size
reader                -4306 B  (-0.8%)    -1197 B  (-0.9%)
plugins                +436 B  (+0.1%)     +139 B  (+0.1%)
plans                  +436 B  (+0.1%)      +51 B  (+0.0%)
home                   +436 B  (+0.1%)     +106 B  (+0.1%)
media                   +19 B  (+0.0%)      -13 B  (-0.0%)
gutenberg-editor        +19 B  (+0.0%)      -13 B  (-0.0%)
post-editor             +16 B  (+0.0%)      +31 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~67 bytes removed 📉 [gzipped])

name                                      parsed_size            gzip_size
async-load-layout-guided-tours                -2319 B  (-14.8%)     -748 B  (-18.9%)
async-load-layout-guided-tours-component      +2032 B   (+2.3%)     +575 B   (+2.6%)
async-load-blocks-inline-help-popover          +436 B   (+0.2%)     +104 B   (+0.2%)
async-load-post-editor-media-modal              +19 B   (+0.0%)      -13 B   (-0.0%)
async-load-design-blocks                        +19 B   (+0.0%)      +15 B   (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@sgomes sgomes force-pushed the update/modularise-guided-tour-state branch from 1b73dcb to 00f07dd Compare June 30, 2020 12:36
@sgomes
Copy link
Contributor Author

sgomes commented Jun 30, 2020

Rebased to fix conflicts.

@sgomes
Copy link
Contributor Author

sgomes commented Jun 30, 2020

@saramarcondes: I think this PR may address part of #43395. See 05c8704.

Edit: Actually, I just noticed that your #43690 addresses the same thing. I'll keep an eye on it and rebase if it makes its way in first 🙂

@sgomes sgomes force-pushed the update/modularise-guided-tour-state branch from 00f07dd to ed4b092 Compare July 8, 2020 14:31
@sgomes
Copy link
Contributor Author

sgomes commented Jul 8, 2020

Rebased to fix conflicts.

Copy link
Collaborator

@wp-desktop wp-desktop left a comment

Choose a reason for hiding this comment

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

WordPress Desktop CI Failure for job "wp-desktop-source".

@sgomes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

@belcherj
Copy link
Contributor

belcherj commented Jul 8, 2020

The above failure of the desktop tests is valid.

@sgomes
Copy link
Contributor Author

sgomes commented Jul 8, 2020

Looks like further usage of the imports being changed has popped up since this PR was opened. Taking another pass.

@sgomes
Copy link
Contributor Author

sgomes commented Jul 8, 2020

Actually, it was a bad merge from the earlier rebase. Fixed.

@sgomes
Copy link
Contributor Author

sgomes commented Jul 8, 2020

Thank you for verifying the failure before I even had a chance to notice it, @belcherj! 😄

@sgomes sgomes requested a review from wp-desktop July 8, 2020 15:10
@wp-desktop wp-desktop dismissed their stale review July 8, 2020 15:10

ci/wp-desktop passing, closing review

@sgomes
Copy link
Contributor Author

sgomes commented Jul 9, 2020

Looks like everything is working correctly now. Any chance of a review before new conflicts pop up?

@sgomes sgomes force-pushed the update/modularise-guided-tour-state branch from f63b13f to d398f1f Compare July 16, 2020 14:25
@sgomes
Copy link
Contributor Author

sgomes commented Jul 16, 2020

Rebased again to fix new conflict.

Copy link
Collaborator

@wp-desktop wp-desktop left a comment

Choose a reason for hiding this comment

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

WordPress Desktop CI Failure for job "wp-desktop-source".

@sgomes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

@nsakaimbo
Copy link
Contributor

Looks like a legitimate failure in the wp-desktop source build:

$ check-npm-client && node ${NODE_ARGS:---max_old_space_size=8192} ./node_modules/webpack/bin/webpack.js --config client/webpack.config.js --display errors-only && yarn run build-languages-if-enabled
 
ModuleNotFoundError: Module not found: Error: Can't resolve 'state/ui/guided-tours/actions' in '/home/circle

@sgomes
Copy link
Contributor Author

sgomes commented Jul 16, 2020

I probably missed something in the merge conflict. Taking a look.

@sgomes sgomes requested a review from rcanepa July 16, 2020 15:02
@wp-desktop wp-desktop dismissed their stale review July 16, 2020 15:06

wp-desktop ci passing, closing review

@sgomes
Copy link
Contributor Author

sgomes commented Jul 16, 2020

Merge conflict properly resolved, ready for review again.

@lsinger
Copy link
Contributor

lsinger commented Jul 20, 2020

@sgomes thanks for your work on this. It's been some time since I've contributed to Calypso, so I can't speak to the actual code changes.

Guided tour state is always loaded on (logged-in) boot, and that appears to be an intentional architectural decision

Yes, Guided Tours needs to be able to initiate a tour based on certain properties. E.g., the media basics tour should be triggered for all new users on a desktop viewport size when they visit /media (and they haven't seen the tour yet).

The tours triggered by a button click in the getting started list seem to work fine. However, the media basics tour didn't trigger for me initially. It only triggered when I edited the URL to say /media exactly (without the added site slug) and hit enter. I'm not sure what then really triggered the tour starting -- the changed URL or simply the reload.

In any case, a new user visiting the media library for the first time should be seeing the tour automatically trigger when visiting from a desktop-sized viewport.

@sgomes
Copy link
Contributor Author

sgomes commented Jul 28, 2020

Hey @lsinger !

Sorry I've only been able to get back to this now, as I took a shift into other projects. Sorry about the ping, please let me know if I should be looping in someone else!

I'm not entirely clear from your comment whether there is an issue with this PR or with the media guided tour in general? In any case, I can't seem to reproduce; the media tour appears to work for me in a newly created user, both in production and on this branch.

@lsinger
Copy link
Contributor

lsinger commented Jul 28, 2020

Sorry I've only been able to get back to this now, as I took a shift into other projects. Sorry about the ping, please let me know if I should be looping in someone else!

Nobody has been working on Guided Tours for quite some time, I think, so I'm not sure. Maybe @jsnajdr? He's in the list of requested reviewers already, though.

I'm not entirely clear from your comment whether there is an issue with this PR or with the media guided tour in general? In any case, I can't seem to reproduce; the media tour appears to work for me in a newly created user, both in production and on this branch.

OK, good to hear -- possibly something went wrong on my side then. 👍

@sgomes sgomes force-pushed the update/modularise-guided-tour-state branch from b56b90f to 138a9e9 Compare July 28, 2020 11:52
@sgomes
Copy link
Contributor Author

sgomes commented Jul 28, 2020

Everything is green after a rebase.

@sarayourfriend
Copy link
Contributor

please smoke test guided tour functionality and ensure that it continues to work as before

@sgomes Do you have a link for how to do activate the guided tour? I'd love to review this PR for you to get it out of your hair but I'm not familiar with the feature.

@sgomes
Copy link
Contributor Author

sgomes commented Jul 29, 2020

@sgomes Do you have a link for how to do activate the guided tour? I'd love to review this PR for you to get it out of your hair but I'm not familiar with the feature.

Hey Sara! That would be amazing, thank you! 😄 There are probably better ways of doing this, but the only reliable way that I know of triggering a guided tour is by creating a new account, creating a free blog on it, and then visiting /media on it (for the media tour; there are others, but this is the one we've been discussing).

A small popup should appear a short time after loading /media in those circumstances on the live branch.

@lsinger
Copy link
Contributor

lsinger commented Jul 29, 2020

@saramarcondes @sgomes Guided Tours has a mechanism for forcing a tour to start. You need to find the tour name, which is usually in camelCase in the tour's configuration file. Then add a query parameter with a key of tour and a value of the tour name.

E.g., when you replace SLUG with one of your sites's slugs here, you should be able to trigger the tour called mediaBasicsTour:

https://wordpress.com/media/SLUG?tour=mediaBasicsTour

Edit: the name is defined in the meta.js file of the tour, e.g., here: https://github.com/Automattic/wp-calypso/blob/master/client/layout/guided-tours/tours/media-basics-tour/meta.js

@sgomes
Copy link
Contributor Author

sgomes commented Jul 29, 2020

Thank you, @lsinger, I knew there had to be a better way! 😄

Copy link
Contributor

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

Armed with @lsinger's helpful advice I was able to test a guided tour and it worked great. Code of course LGTM 🤠

@sgomes
Copy link
Contributor Author

sgomes commented Jul 29, 2020

Woohoo, thank you @saramarcondes! 😄 I'll get this merged tomorrow morning my time, when things are quieter 🙂

@sgomes sgomes merged commit f30232b into master Jul 30, 2020
Modularized state migration automation moved this from In progress to Done Jul 30, 2020
@sgomes sgomes deleted the update/modularise-guided-tour-state branch July 30, 2020 11:01
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants