GuidedTours: Add themes tour #6472
Conversation
|
Rebased. |
|
'Rebased' |
|
Rebased against master. |
ehg
force-pushed
the
add/themes-tour
branch
2 times, most recently
from
9d190f5 to
f4f01f5
Compare
|
Annnd another rebase! |
|
Rebased. Flipping over for review. Some of the GT tests fail, should we be testing a "live" tour in unit tests @mcsf?
|
ehg
added
[Status] Needs Review
|
UX obs.: should we take users back to where they started, i.e. have them tap All Themes from the sheet view? |
Good point. As per Slack, let's keep some higher-level tests that seek to keep GT from breaking in production, and make the remaining tests consume a fixture GT config. |
|
Pushed 6aa07cb, which fixes tests by running them against a fake config. |
| meta: { | ||
| version: 'test', | ||
| path: '/', | ||
| context: state => isNewUser( state ), |
There was a problem hiding this comment.
Worth noting that the tests were failing when this line read:
context: isNewUserI believe this is due to circular dependencies between test/config and selectors. Wrapping isNewUser in a lambda solves this by giving us lazy evaluation of isNewUser.
There was a problem hiding this comment.
That's pretty interesting. I seem to remember circular dependencies resolving to null. I think we can just deal with the wrapping now :)
There was a problem hiding this comment.
Yep. IIRC yesterday I was getting undefined, rather than null, but yes.
It's probably fine in here. |
|
I was just thinking, if we haven't got the step copy nailed down, we shouldn't be translating anything in this tour. Sound sensible? |
Done. |
|
Removed |
Absolutely, good one. |
| type: 'ActionStep', | ||
| target: '.themes__search-card .section-nav__mobile-header', | ||
| placement: 'above', | ||
| showInContext: () => isMobile(), |
There was a problem hiding this comment.
(minor) Does showInContext: isMobile break?
There was a problem hiding this comment.
I don't think so, but mobile is pretty broken anyway. I'm considering removing it for now, and adding a ! isMobile() to the tour context. Thoughts?
There was a problem hiding this comment.
Yeah, I'm not opposed. However, it'll be nice to have something in mobile soon enough so that we can tackle the expectable oddities/jankiness early in development.
There was a problem hiding this comment.
I've removed it for now: 87ebbe4
Yeah, I suspect we should go for a separate tour as previously discussed.
|
I think this is ripe for merging. |
|
Rebased via reset. Also, I'm slapping 5a9c1ac in here to disable the |
I'm hardly a source of sanity, but I had another look — |
🚢 🚀 !
mcsf
added
[Status] Ready to Merge
and removed
[Status] Needs Review
A basic themes tour. Used to help us develop the GuidedTours framework. Initially, we'll just push this out into wpcalypso, to get some more eyes on the steps, their flow, and their content.
TODO:-
Test via: http://calypso.localhost:3000/design/[somesite]?tour=themes
Test live: https://calypso.live/?branch=add/themes-tour