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

Guided Tours: Add Jetpack Sign-In Tour #25864

Merged
merged 2 commits into from
Jul 9, 2018
Merged

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jul 4, 2018

Screenshots:

image

Success Message:

image

To test:

  • Land at calypso.localhost:3000/checklist/<jpSite>
  • Click the 'Do it!' button next to the 'WordPress.com sign in' item.
  • Lo and behold the Guided Tour
  • Enable WordPress.com sign in, as suggested by the Guided Tour thingy
  • Find a gratifying success notice ✔️
  • Verify that 'Yes' button takes you back to the checklist, 'No' drops you off where you are.

Original Mockup (from p6TEKc-1UX-p2):

04-wpcom-signin

Fixes #25459.

@matticbot
Copy link
Contributor

@ockham ockham force-pushed the add/jetpack-sso-guided-tour branch from 88294ca to 08ad11e Compare July 4, 2018 12:33
@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Jul 4, 2018
@ockham ockham requested review from joanrho and a team July 4, 2018 12:36
@joanrho
Copy link
Contributor

joanrho commented Jul 6, 2018

Thanks @ockham! I'm seeing a bit of wonkiness with the positioning of the guided tour: I haven't set up Backups and Scanning for this particular test site yet, so the guided tour bubble landed on the Spam Filtering card. Would it be possible to change that so that it affixes to the Sign-in card instead? I believe this happened for a previous tour we worked on...did you resolve that by specifying the class name or something?

Also, the guided tour is currently positioned left: 339px;, which positions the caret just to the right of the toggle. Can we center this instead by making it left: 319px;? Before/after: https://cloudup.com/cHD6Eh7VgXp

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Code looks good and this tests as described. Nice work 👍

</h1>
<p>
{ translate(
'You can now sign into your Jetpack site with your WordPress.com account. Would you like to continue setting up the security essential features for your site?'
Copy link
Member

Choose a reason for hiding this comment

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

This line is too long now - care to split it into 2 lines? There are 2 sentences, so it should make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird that eslint didn't complain about this...

Copy link
Member

Choose a reason for hiding this comment

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

Weird that eslint didn't complain about this...

#24635 added prettier eslint config. Line length is handled by prettier and max-length is no longer enforced.

Prettier doesn't break string and it's still nice to break lines that get too long 👍

@ockham
Copy link
Contributor Author

ockham commented Jul 6, 2018

Thanks @ockham! I'm seeing a bit of wonkiness with the positioning of the guided tour: I haven't set up Backups and Scanning for this particular test site yet, so the guided tour bubble landed on the Spam Filtering card. Would it be possible to change that so that it affixes to the Sign-in card instead? I believe this happened for a previous tour we worked on...did you resolve that by specifying the class name or something?

Hmm, that's weird. The className should be specific enough (.sso__card .form-toggle__switch).

I remember the other issue, but I'm afraid I essentially forgot to address it 😳 -- I probably let this slip thru the cracks since I couldn't repro it. I can't reproduce this one either in Chromium, so it sounds a bit like this could be a browser specific bug with Guided Tours. What browser are you using to test this @joanrho?

@ockham
Copy link
Contributor Author

ockham commented Jul 6, 2018

Also, the guided tour is currently positioned left: 339px;, which positions the caret just to the right of the toggle. Can we center this instead by making it left: 319px;? Before/after: https://cloudup.com/cHD6Eh7VgXp

Yeah, this is kinda annoying. Guided Tours positioning is based on element selectors (.sso__card .form-toggle__switch), and some more 'qualitative' args (arrow="top-left", placement="below"). You'd think that'd do a better job:

position

Maybe I can use the additional style arg to tweak this...

@joanrho
Copy link
Contributor

joanrho commented Jul 6, 2018

@ockham I'm using Chrome. Pinging @Automattic/jetpack-design to see if anyone else from the team can repro this bug.

@keoshi
Copy link
Contributor

keoshi commented Jul 9, 2018

Same here: using Chrome and seeing the guided tour step slightly nudged to the right, relative to the actual toggle.

image

@keoshi
Copy link
Contributor

keoshi commented Jul 9, 2018

Unfortunately it doesn't seem like an easy problem to fix because as @ockham mentions, its position is calculated from the picked selector, so the left values you're seeing @joanrho are generated dynamically and change depending on the screen resolution you use.

transform: translateX(-16px) would be ideal here but doesn't work because it has a fixed position.

margin-left: -16px seems to do the trick, but it's not pretty at all.

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 9, 2018
@jeffgolenski
Copy link
Member

Just gave this a whirl. Can confirm the guided tour is appearing slightly off.

screen shot 2018-07-09 at 3 00 03 pm

@ockham
Copy link
Contributor Author

ockham commented Jul 9, 2018

Thanks all! I've filed #25932 and 25933, respectively, to keep track of the issues found by @joanrho. Going to merge this PR to unblock the Guided Tour.

@ockham ockham merged commit 5e62832 into master Jul 9, 2018
@ockham ockham deleted the add/jetpack-sso-guided-tour branch July 9, 2018 21:01
@matticbot matticbot removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 9, 2018
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.

7 participants