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

Add new connect splash screen content. #9138

Merged
merged 12 commits into from
Jun 5, 2018
Merged

Conversation

zinigor
Copy link
Member

@zinigor zinigor commented Mar 23, 2018

Fixes #8900 and #8617

This still needs some work:

  • URLs for white buttons needed.
  • Scrolling doesn't work because of the absolutely positioned element.
  • The 'not now' button doesn't do anything.
  • Paddings and margins seem to be off.

I was hoping I could get some help with this from the design team.

screenshot from 2018-03-23 22-35-20

Changes proposed in this Pull Request:

Testing instructions:

  • Deactivate Jetpack from the plugins menu.
  • Activate Jetpack again, see the splash screen

Proposed changelog entry for your changes:

  • Refreshed the connect splash screen to help new users learn more about Jetpack.

@zinigor zinigor added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Status] Needs Design Review Design has been added. Needs a review! [Focus] FixTheFlows labels Mar 23, 2018
@zinigor zinigor requested a review from a team as a code owner March 23, 2018 19:37
@ghost ghost removed the [Status] In Progress label Mar 23, 2018
@joanrho
Copy link
Contributor

joanrho commented Mar 23, 2018

URLs for white buttons needed.

Chat with us: https://jetpack.com/contact-support/
Search our support site: https://jetpack.com/support/

@jeffgolenski
Copy link
Member

jeffgolenski commented Mar 23, 2018

Just some quick feedback!

  • There's like 6 different text styles on this one page, let's cut that back!
  • Jetpack Logo is called with an img tag — we can just inline that SVG to reduce a request!
  • Line-height of text in footer of the page needs to be taller. Line that starts with "Connect to, or create"
  • I'd like to call in @nicoleckohler for a quick review of the content here, just in case of any grammatical or flow issues
  • Need to make some adjustments for mobile. See image below. The illustrations are super small
  • The scrolling issue as discussed in slack!

screen shot 2018-03-23 at 4 43 47 pm

@joanrho The only goal of this page is to get people to connect. Do you think the "chat with us" or "search our support site" buttons are necessary?

@joanrho
Copy link
Contributor

joanrho commented Mar 23, 2018

@jeffgolenski Thanks for the review and feedback! I'll get on the styling once we have the scrolling worked out.

As for the "chat with us" and "search our support" site buttons, that section already exists in the current longer disconnection full-screen banner, so my version updates that as a way for uncertain users to reach out for help before deciding. Here's what the current (old) one looks like:

01_wpadminjetpackdash_disconnected_old

I don't think we should nix this just yet, but we may decide to later on if we see that few users click through to those support options.

@zinigor
Copy link
Member Author

zinigor commented Mar 26, 2018

Thanks for the review @joanrho and @jeffgolenski !

@jeffgolenski about this:

Jetpack Logo is called with an img tag — we can just inline that SVG to reduce a request!

I'd argue here that one more request doesn't hurt that much, while we have the benefit to make changes to the logo in one place if needed. This PR also makes the settings masthead include that logo as a file.

@zinigor
Copy link
Member Author

zinigor commented Mar 26, 2018

This is ready for review again, @jeffgolenski . Thanks for the feedback!

Copy link
Contributor

@joanrho joanrho left a comment

Choose a reason for hiding this comment

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

Thanks for this, @zinigor! I added some minor styling to the "not now" dismiss text and increased the bottom padding.

@rickybanister
Copy link

Did the font style issues get resolved? I know @zinigor is on a linux machine, but we're seeing both system fonts and helvetica in his screenshots.

@oskosk oskosk added this to the 6.1 milestone Apr 11, 2018
@@ -14,6 +14,7 @@ import analytics from 'lib/analytics';
*/
import { getSiteConnectionStatus } from 'state/connection';
import { getCurrentVersion, userCanEditPosts } from 'state/initial-state';
import { imagePath } from 'constants/urls';
Copy link
Member

Choose a reason for hiding this comment

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

This is not defined when we're building the static.html file for the loading screen, so the console is always giving me a 404 for this path, even though the logo is showing after it loads.

@rickybanister
Copy link

rickybanister commented Apr 24, 2018

Some things I noticed:

  • The footer area is acting strange, the gray is partially covering the wp footer and the white overlay ends at a strange place 200px from the bottom:

image

  • The typography all seemed small. On Jetpack.com the h1 and h2 are 2.5em and compute to 35px. Here they are 1.35em and compute to 22px. The terms of service text is 0.6875rem and computes to 11px which is too small. Body copy on Jetpack.com's homepage is 1.25em and here it's not set so it defaults to the p style which is 13px. I suppose we don't need to match jetpack.com exactly, but bumping up the headlines, paragraph styles, and small text would help.

image

  • This PR only covers the pop up splash content, not the disconnected dashboard (in the react codebase) stuff. We still need to take care of that.

@dereksmart dereksmart removed this from the 6.1 milestone Apr 24, 2018
@dereksmart
Copy link
Member

I added a commit that renders the svg inline in the masthead, which fixes the undefined problem.

It looks a little choppy to me though.

screen shot 2018-04-24 at 5 47 02 pm

@rickybanister
Copy link

Let's merge this as is and address the choppy logo and other to-dos in another PR, is that alright @dereksmart?

@jeffgolenski can you take a look at the logo svg and the other checkboxed items I posted?

@joanrho
Copy link
Contributor

joanrho commented May 8, 2018

The footer area is acting strange, the gray is partially covering the wp footer and the white overlay ends at a strange place 200px from the bottom:

I'm not seeing this @rickybanister. Can you test again and see if you are?

@joanrho
Copy link
Contributor

joanrho commented May 9, 2018

I've updated the styling so it better matches the hierarchies in my original mockups and removed the word "fascinating" from our TOS line in functions.global.php:

before after

Ready for review and to pass on to whoever for next steps (disconnected dashboard?).

cc @zinigor @dereksmart @rickybanister

@rickybanister
Copy link

That looks much better.

Let's get this merged!

@zinigor @dereksmart, what's the best way to copy this to the dashboard, should we rewrite it and duplicate it? Are there any other viable solutions with less duplication? Can we use some interpretive thing for the links that get 'pre-processed' by the php and react code so we can reuse some meta template for the content and copy?

@dereksmart
Copy link
Member

@rickybanister -- the good news is that @oskosk is currently working on a solution so we can reuse components in html/templates, so we only have the write them once.

The bad new is that to take advantage of that, this whole PR would have to be re-written in React.

@keoshi
Copy link
Contributor

keoshi commented Jun 4, 2018

@zinigor This seems to be in great shape to be closed—want to go ahead and merge it? Thanks!

@keoshi keoshi added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress [Status] Needs Design Review Design has been added. Needs a review! labels Jun 4, 2018
@zinigor zinigor added this to the 6.3 milestone Jun 5, 2018
@zinigor zinigor merged commit e04bcd9 into master Jun 5, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 5, 2018
@zinigor zinigor deleted the add/connect-splash-content branch June 5, 2018 16:34
oskosk added a commit that referenced this pull request Jun 26, 2018
oskosk added a commit that referenced this pull request Jun 26, 2018
* Set boilerplate for 6.3 in readme.txt

* Add changelog for 6.2.1 to changelog.txt

* Set boilerplate for 6.3 in to-test.md

* Sync Stable tag to 6.2.1

* Add #5647 to changelog

* Add #7956 to changelog

* Add #9138 to changelog

* Add #9573 to changelog

* Add #9728 to changelog

* Add #9732 to changelog

* Add #9752 to changelog

* Add some testing instructions

* Add #9823 to changelog

* Add #9780 to changelog

* Add testing instructions for #9780

* Add #9525 to changelog

* Add #9782 to changelog

* Add #9828 to changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] FixTheFlows [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants