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

Update login screen for existing logged in user #40125

Merged
merged 13 commits into from Mar 20, 2020

Conversation

razvanpapadopol
Copy link

@razvanpapadopol razvanpapadopol commented Mar 13, 2020

Changes proposed in this Pull Request

  • increase the gravatar image size and remove login form and other links from initial view
  • display Log in with another account text below with a link to show login form and everything else

image

Testing instructions

  • Stay logged-in
  • Open /log-in

Fixes (together with #40314): #39988
Related: #35309

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Mar 13, 2020

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

App Entrypoints (~144 bytes added 📈 [gzipped])

name         parsed_size           gzip_size
entry-login       +698 B  (+0.1%)     +144 B  (+0.1%)

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

Async-loaded Components (~150 bytes added 📈 [gzipped])

name                      parsed_size           gzip_size
async-load-design-blocks       +705 B  (+0.0%)     +150 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.

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Tests well. Tested:

  • connecting Jetpack, no signup form presented when already logged in
  • open /log-in and /log-in/en as authenticated, non-authenticated, and 2fa users
  • open /log-in?site=SITE_SLUG same as above
  • Tested that actually logging in with another user in this scenario worked

With ?site=SITE_SLUG still works (my name is the site name in this case):

image

Non-2FA user, no first+lastname set, english locale:

image

With 2FA user in different locale:

image

(These texts just need translating)

@simison simison added the [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging label Mar 16, 2020
@simison
Copy link
Member

simison commented Mar 16, 2020

Added [Status] String Freeze label to ensure these are sent to translation before we merge this.

@simison
Copy link
Member

simison commented Mar 16, 2020

There's a short gap when the login form is rendered while data loads and this avatar step isn't loaded:

https://cloudup.com/cL5OLeTemOW

Assuming that would be the case even if we would constantly show the form, this isn't a big change in behavior.

To prevent this, we should hide the login form until API response tells us that the user in-fact is logged in. That would delay the login form visibility for many, while this visual jump is visible to few, so I think it's acceptable.

@Automattic/team-calypso @sgomes would you like to review/test this before merging as it's touching the login landingpage?

@sgomes
Copy link
Contributor

sgomes commented Mar 16, 2020

@Automattic/team-calypso @sgomes would you like to review/test this before merging as it's touching the login landingpage?

Thanks, @simison! I think this PR mostly deals with UI questions, not performance issues, so I'm going to leave any considerations to more knowledgeable folks in that area. There seems to be nothing concerning, perf-wise, from a brief glance at the code and the ICFY results.

@simison simison 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 Mar 16, 2020
@simison
Copy link
Member

simison commented Mar 16, 2020

@sgomes sure, no worries 👍 Just pinged you because you introduced the initial feature in #35309 and wanted to make sure I'm not missing anything. :-)

@sgomes
Copy link
Contributor

sgomes commented Mar 16, 2020

@sgomes sure, no worries 👍 Just pinged you because you introduced the initial feature in #35309 and wanted to make sure I'm not missing anything. :-)

Thank you for keeping me in the loop, @simision! 🙂 It's nice to see folks taking the idea and improving it further 🎉

@simison
Copy link
Member

simison commented Mar 16, 2020

Some of the test failures seem related (test runner cannot see an element with #usernameOrEmail) and might need adjusting before the merge.

@amamujee
Copy link

Not sure if it's something weird with my browser but some of the spacing looks off and the old login screen appeared for a second before the new shiny one.
Log_In_—_WordPress_com

@lancewillett
Copy link
Contributor

lancewillett commented Mar 16, 2020

Looking good! Quick test in Firefox latest stable on macOS, using a test user (non-Automattician).

Before After

Click thumbnails for large view.

@razvanpapadopol
Copy link
Author

Thanks for checking this out @amamujee !

some of the spacing looks off

Looks like the new CSS definitions included in this PR are ignored by your browser. I'm not sure if there can be a caching issue given the hash in the live URL. Anyway if this doesn't go away with a hard refresh (CMD + shift + R in Chrome), please share the browser version. I've tested in latest Chrome/Firefox/Safari including mobile and it looks fine.

old login screen appeared for a second before the new shiny one.

This is the UI issue @simison mentioned earlier and it's illustrated in this demo: https://cloudup.com/cL5OLeTemOW
We need to decide if the jump is acceptable from UX perspective because hiding the login form until user data is available would cause a delay (1 second or more depending on the bandwidth) in showing the whole login block. cc @Automattic/dotcom-create-design

@simison
Copy link
Member

simison commented Mar 16, 2020

We need to decide if the jump is acceptable from UX perspective because hiding the login form until user data is available would cause a delay (1 second or more depending on the bandwidth) in showing the whole login block.

We could ease it a bit by fading the avatar into the view, so it'll look more intentional instead of the sudden jump.

@lancewillett
Copy link
Contributor

Comment on the idea to improve the jump. Let's ship without that for now. Because this change was meant to be a quick adjustment to a much-used screen, speed is more important than perfect.

The change is already a major improvement for users, and those additional improvements fall into the nice-to-have category, "maybe later."

@amamujee
Copy link

amamujee commented Mar 17, 2020

How hard is the fade to implement?

When I tried it, I personally felt it was pretty jarring and also like there was a bug or screen that I missed that made me feel anxious. If the fade is not easy, I think the delay is a better UX than the disappearing screen for 1 second (although I've not tried it side by side).

I think @razvanpapadopol and @simison should make the call here though.

client/blocks/login/continue-as-user.jsx Outdated Show resolved Hide resolved
client/blocks/login/continue-as-user.jsx Outdated Show resolved Hide resolved
client/blocks/login/index.jsx Outdated Show resolved Hide resolved
@razvanpapadopol razvanpapadopol requested a review from a team as a code owner March 17, 2020 18:20
@razvanpapadopol
Copy link
Author

Some of the test failures seem related (test runner cannot see an element with #usernameOrEmail) and might need adjusting before the merge.

Yes, some of the tests were expecting the login form to always be there. Fixed in 898153b

cc @Automattic/e2e-test-reviewers

@lancewillett
Copy link
Contributor

@razvanpapadopol What do you think about @simison's suggestion to hide the login form until the API request is successful?

Now that I understand the issue, I can see how the login form hide / show delay even on a fast connection is not ideal.

Another suggestion is to use a loading icon and then once ready, show the correct version of the page.

Răzvan Papadopol and others added 7 commits March 19, 2020 14:28
remove empty space

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
- Remove handleChangeAccount const from ContinueAsUser
- add loginAsAnotherUser id to ContinueAsUser component
- add try/catch block in login method of LoginPage
- don't wait for #loginAsAnotherUser
@razvanpapadopol razvanpapadopol 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 Mar 19, 2020
@razvanpapadopol razvanpapadopol dismissed bsessions85’s stale review March 19, 2020 12:55

fixed the failing e2e tests by implemented suggested change. Thanks!

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Looks good and works well. 👍

Once translations are done, we can switch to final texts in a separate PR.

Thanks for tackling this one!

@razvanpapadopol
Copy link
Author

Once translations are done, we can switch to final texts in a separate PR.

Merging this one now and once translations are ready we can do the switch on #40314 🚀

@razvanpapadopol razvanpapadopol merged commit a115134 into master Mar 20, 2020
@razvanpapadopol razvanpapadopol removed [Status] Needs e2e Testing [Status] Ready to Merge [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging labels Mar 20, 2020
@razvanpapadopol razvanpapadopol deleted the update/login-continue-as-user branch March 20, 2020 05:31
@lancewillett
Copy link
Contributor

lancewillett commented Mar 20, 2020

🎉 Testing it live on iPhone XS with mobile Safari — smooth transition and works as expected.

@olaolusoga
Copy link

Late design feedback, it'd be great to get these changes in before pushing:

After looking at Non-2FA user, no first+lastname set, english locale: screenshot, made small adjustments to address translation, and longer usernames.

Design feedback:

  • Bump down the gravatar width/height to 110px—it's currently at 200px.
  • Add 1px border to gravatar. Border color: #A7AAAD
  • Gravatar should be clickable—it isn't at the moment.
  • Can we make sure everything is center.
  • Remove username from button, and add below gravatar image (see mock)
  • Update copy from "Not [username]? Login in with a different account" to "Not you?
    Log in with another account" (see screenshot)

Updates
Only first name
short username

First and last name
longer username

no first and last name, no set gravatar image
long username, no image

Updates map closely to mobile view. Here's mobile view mock (for reference on things to adjust):
X - 1

@lancewillett
Copy link
Contributor

Work still needed to wrap up the feedback: #40314

@roo2
Copy link
Contributor

roo2 commented Mar 30, 2020

created #40556 to capture the design feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants