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

GuidedTours: Add steps for site preview #5553

Merged
merged 1 commit into from
May 26, 2016

Conversation

ehg
Copy link
Member

@ehg ehg commented May 24, 2016

Closes #5120

Now that we're able to preview an iframed view of the current site by clicking on the current 'site card'…

screen shot 2016-05-24 at 16 18 51

…we want to modify the main tour to include steps to preview a site. This has the benefit of showing people their sites early on after signup.

preview-steps

TODO:

  • Bump tour version (not here)
  • Deploy in line with ABtest date/percentage change PR GuidedTours: Update MVP tour variants to 15% each #5450 (not here)
  • Decide what to do if no selectedSite is available
  • Fix tests
  • if the wait is over 2-3 seconds ... (e.g. when no site selected)
    • end the tour
    • add a debug() message
    • and maybe a tracks/MC stat
  • enable conditional branching to keep the tour working even when in-Calypso preview is disabled, e.g. next: [ { 'config( 'preview-layout' )' : 'preview' }, { 'default': 'themes'} ]
  • Check mobile widths

To test:

  1. Go to http://calypso.localhost:3000/?tour=main
  2. Check that the steps are positioned correctly, and the preview opens and closes
  3. Check that it works on mobile widths
  4. Go to http://calypso.localhost:3000/stats/day?tour=main (hopefully this will force all-sites mode) — check that the tour errors gracefully
  5. DISABLE_FEATURES=preview-layout make run — check http://calypso.localhost:3000/?tour=main doesn't include the preview steps

Props @lsinger @mcsf

@ehg
Copy link
Member Author

ehg commented May 24, 2016

From @mcsf:

Feel free to revert the changes in the selector, but:

  • /stats/day/mysite.wordpress.com/?tour=main works
  • /?tour=main works
  • /stats/day/?tour=main DOESN'T work

The reason #2 works is that it loads the Reader first, and tapping My
Sites after that will try to take the user to a specific site (to me at
least), whereas in #3 we explicitly go to My Sites with no site
selected, thus the sidebar has no site icon.

Also:

  • wait.js: my testing shows that the switch to the Preview step is only
    'delayed' once or twice, but even so I decided to implement a rapidly
    growing delay as a defensive practice (e.g., in the Menus: Alignment issue with undo link in is-info notice after deleting menu #3 above, wait
    will keep trying indefinitely, but thanks to the growing delay that
    doesn't become a perf concern).
  • That said, we could also set some break condition, like a hardcoded
    maximum delay (or even a function if we feel that need) after which
    wait stops trying.

@ehg ehg force-pushed the add/guidestours-preview-steps branch from 705b868 to 2c83628 Compare May 25, 2016 10:13
@ehg
Copy link
Member Author

ehg commented May 25, 2016

Rebased.

@ehg
Copy link
Member Author

ehg commented May 25, 2016

Works well with responsive mode in Chrome for me! Nice.

@lsinger
Copy link
Contributor

lsinger commented May 25, 2016

Works well with responsive mode

Yeah, I was quite surprised actually.

@ehg ehg assigned lsinger and unassigned ehg May 25, 2016
@lsinger lsinger force-pushed the add/guidestours-preview-steps branch from 4046e60 to c0d8c37 Compare May 25, 2016 14:33
@lsinger
Copy link
Contributor

lsinger commented May 26, 2016

Decide what to do if no selectedSite is available

Should we maybe just skip steps if the waiting function takes too long?

@ehg
Copy link
Member Author

ehg commented May 26, 2016

Should we maybe just skip steps if the waiting function takes too long?

We could end the tour if the wait is over 2-3 seconds, adding a debug() message and maybe a tracks/MC stat — so we can see when it happens while developing, and in production.

@lsinger
Copy link
Contributor

lsinger commented May 26, 2016

We could end the tour if the wait is over 2-3 seconds

Seems better. Plus maybe a notice for the user.

@ehg ehg added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels May 26, 2016
@ehg
Copy link
Member Author

ehg commented May 26, 2016

LGTM. Squash and merge!

@lsinger
Copy link
Contributor

lsinger commented May 26, 2016

@ehg checked on Firefox and Chrome on an Android device, I checked on Safari on an iPhone. All work fine.

This adds a tour step that nudges the user to open their site's
preview, and another one for closing it again. These steps are skipped
if `preview-layout` is disabled.

As part of this -- to guard against problems when there was no site
selected, so that preview wouldn't work -- this also introduces a
timeout for finding the next step's target, if there is one. If this
takes longer than a few seconds, we end the tour gracefully, tell the
user, and record a tracks event with an error message.
@lsinger lsinger force-pushed the add/guidestours-preview-steps branch from 2c254a0 to 9a417d3 Compare May 26, 2016 20:56
@ehg ehg added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 26, 2016
@lsinger lsinger merged commit da8088e into master May 26, 2016
@lsinger lsinger deleted the add/guidestours-preview-steps branch May 26, 2016 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GuidedTours: add a site preview step
3 participants