Laushinka/login 6826 #7046

laushinka · 2021-12-02T23:23:10Z

Description

Unlogged visitors using a prefix are shown:

No welcome section
More relevant text
A button to continue only with the corresponding Git provider

Related Issue(s)

Fixes #6826
Depends on #7081

How to test

Append a repo URL to https://laushinka-login-6826.staging.gitpod-dev.com/#
See the changed page

Release Notes

Unlogged visitors using a prefix will be shown a more direct login page.

Documentation

jankeromnes

Many thanks for improving the login screen UX!

Unfortunately, there still seems to be some uncertainty around what exactly the page should look like in #6826, but I guess this PR can be adjusted when the final design is agreed upon.

(I have a slight suspicion that we might move to something like #6826 (comment) but with a dropdown element to the Continue with GitHub button as shown in PROPOSAL in #6826 (comment) -- but let's see).

I gave this a try, and it seems to work as intended in various scenarios. 👍

Except, however, that it always shows the welcome screen for me (while the PR description says "1. No welcome section"). Maybe that's unintended?

No repo	With GitHub repo	With GitLab repo

Also left some minor thoughts in the code, but please let me know if you'd like a more thorough code review at this point -- happy to do that! 😊

components/dashboard/src/Login.tsx

components/dashboard/src/App.test.ts

laushinka · 2021-12-03T09:50:45Z

Except, however, that it always shows the welcome screen for me (while the PR description says "1. No welcome section"). Maybe that's unintended?

@jankeromnes Oh this shouldn't have happened. I will check, thanks!

please let me know if you'd like a more thorough code review at this point

I'm always happy with that! We can also do it synchronously if that works for you? 🤗

jankeromnes · 2021-12-03T10:02:55Z

please let me know if you'd like a more thorough code review at this point

I'm always happy with that! We can also do it synchronously if that works for you? 🤗

Sure! More than happy to sync on this -- how about this afternoon, e.g. 14:00 or 15:00? (Or please feel free to pick any time you like here 😁)

I often feel like we should do more pair programming at Gitpod, but somehow there is always a bit of friction to set it up, so I end up never doing it. 🙈 Thanks for providing this opportunity! 🙏

laushinka · 2021-12-03T10:06:07Z

I often feel like we should do more pair programming at Gitpod, but somehow there is always a bit of friction to set it up, so I end up never doing it. 🙈 Thanks for providing this opportunity! 🙏

💯💯💯💯💯💯

roboquat · 2021-12-03T10:36:01Z

@Foge9627: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

laushinka · 2021-12-03T12:08:16Z

Except, however, that it always shows the welcome screen for me (while the PR description says "1. No welcome section"). Maybe that's unintended?

@jankeromnes ~~Push fixed!~~ ~~Fish puxed!~~ (I think my brain is ready for the weekend) Pushed fix! We can still go through the code later in the call :)

laushinka · 2021-12-03T16:14:18Z

/werft run

👍 started the job as gitpod-build-laushinka-login-6826.18

laushinka · 2021-12-03T17:44:48Z

/werft run

👍 started the job as gitpod-build-laushinka-login-6826.19

laushinka · 2021-12-03T17:52:42Z

Build finally green again after werft issue fixed. Ready for another code review if you'd like @jankeromnes. (I don't mean this weekend of course 🙈)

jldec · 2021-12-05T23:30:55Z

/werft run

👍 started the job as gitpod-build-laushinka-login-6826.20

jldec · 2021-12-06T00:40:06Z

Thanks @laushinka - nice and clean :)

I'm missing the Gitpod name on the page somewhere.

The "Log in / Sign up" should not be bigger than the line below it, which carries an equally important messge.

I got into trouble trying to login with GitLab (ended up on another login page with 3 buttons) - I would suggest giving the GitLab flow a little more testing.

svenefftinge · 2021-12-06T06:51:41Z

Looks like the version from the first comment was implemented and not the one that I understood was agreed on. Is that intentional?

jankeromnes · 2021-12-06T08:26:03Z

Build finally green again after werft issue fixed.

Hooray! 🙌

Ready for another code review if you'd like @jankeromnes. (I don't mean this weekend of course 🙈)

Thanks Laurie! 🙏😊 Sure, I'm happy to do another code review (could you please re-request review in the top right of this PR? This helps me differentiate PRs between "work in progress" and "blocked until reviewed" 😇). However, I also agree with Sven: Could you first please adjust the design to this version? I agree the issue isn't very clear (so many design variations 🙈) but I think that one is the current best compromise.

laushinka · 2021-12-06T08:36:11Z

Looks like the version from the first comment was implemented and not the one that I understood was agreed on. Is that intentional?

@svenefftinge I can do this version - it did get confusing inside the issue, but I will take the comments in this PR as final, and specify that in the issue as well. cc @jldec to make sure we're aligned.

components/dashboard/src/Login.tsx

laushinka · 2021-12-07T09:59:27Z

Will plug in the experiments code once this is merged.

laushinka · 2021-12-07T10:50:53Z

@geropl Please review the experiments code 🙏🏽

jankeromnes · 2021-12-07T10:51:06Z

Thanks again @laushinka! Taking another look now/soon 😇

(EDIT: Will check the theme fixes when the build is done 😋)

geropl · 2021-12-07T10:53:47Z

components/dashboard/src/experiments.ts

@@ -28,6 +28,7 @@ const Experiments = {
     * Experiment "example" will be activate on login for 10% of all clients.
     */
    // "example": 0.1,
+    "login-from-context-6826": 0.5,


Some documentation incl. a link to this PR would be nice!

Also, side question: Can you manually "force" A or B to test code-dev deployments?

Can you manually "force" A or B to test code-dev deployments?

Sure, you have to manipulate localStorage in your web dev tools. They are configured to be stable atm, so will happily accept your changes.

Update: just recognized that this is not true, actually! And found a bug in the experiment impl (not this PR). Will fix after lunch. 🙃

components/dashboard/src/Login.tsx

geropl · 2021-12-07T10:55:37Z

components/dashboard/src/Login.tsx


    useEffect(() => {
        (async () => {
            setAuthProviders(await getGitpodService().server.getAuthProviders());
        })();
    }, [])

+    if (Experiment.has(experiment)) {


@laushinka My bad, turns our React does not like this: build failed with Line 83:9: React Hook "useEffect" is called conditionally. React Hooks must be called in the exact same order in every component render react-hooks/rules-of-hooks.

Then the if(Experiment.hat... goes into the useEffect, on top-level. 👍

Yes just saw this as well

jankeromnes · 2021-12-07T11:00:50Z

components/dashboard/src/Login.tsx


    useEffect(() => {
        (async () => {
            setAuthProviders(await getGitpodService().server.getAuthProviders());
        })();
    }, [])

+    if (Experiment.has(experiment)) {


React doesn't seem to like the conditional useEffect.

Maybe always run the useEffect, but do if (!Experiment.has(experiment)) { return; } at the top of it?

Yes just realized this too

components/dashboard/src/Login.tsx

geropl · 2021-12-07T12:39:59Z

@laushinka Sorry for the additional confusion - and thx for fixing. 🙏 Here's a follow-up PR to fix other issues as well: #7103

geropl

Cannot test - "max number of users for licence" reached 🤷

But tested before, and code lgtm, so: 👍

roboquat · 2021-12-07T12:49:33Z

LGTM label has been added.

Git tree hash: 26cdb08ca8eb7574a013df588f0031e3827e8a76

roboquat · 2021-12-07T12:49:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Foge9627, geropl

Associated issue: #6826

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/dashboard/OWNERS~~ [geropl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jldec · 2021-12-12T23:46:28Z

See followups #7188 and #7138

roboquat added release-note team: webapp Issue belongs to the WebApp team size/L labels Dec 2, 2021

laushinka force-pushed the laushinka/login-6826 branch from bc90eae to 7a8226e Compare December 2, 2021 23:25

laushinka requested review from gtsiolis, jldec and jankeromnes December 2, 2021 23:27

laushinka mentioned this pull request Dec 2, 2021

Customize the Login page for unauthenticated users arriving from an "Open in Gitpod" context #6826

Closed

jankeromnes reviewed Dec 3, 2021

View reviewed changes

components/dashboard/src/Login.tsx Outdated Show resolved Hide resolved

components/dashboard/src/App.test.ts Outdated Show resolved Hide resolved

Foge9627 approved these changes Dec 3, 2021

View reviewed changes

laushinka force-pushed the laushinka/login-6826 branch 2 times, most recently from 013a256 to e0ad6a6 Compare December 3, 2021 11:56

Foge9627 mentioned this pull request Dec 3, 2021

[APPROVALNOTIFIER] This PR is **NOT APPROVED** #7055

Closed

roboquat added size/M and removed size/L labels Dec 3, 2021

laushinka force-pushed the laushinka/login-6826 branch 2 times, most recently from 3293f6b to 09fbb2d Compare December 3, 2021 15:15

geropl reviewed Dec 7, 2021

View reviewed changes

components/dashboard/src/Login.tsx Show resolved Hide resolved

laushinka force-pushed the laushinka/login-6826 branch from cf2f6c5 to df25033 Compare December 7, 2021 10:49

roboquat added size/L and removed size/M labels Dec 7, 2021

laushinka requested a review from geropl December 7, 2021 10:50

geropl reviewed Dec 7, 2021

View reviewed changes

components/dashboard/src/Login.tsx Outdated Show resolved Hide resolved

geropl reviewed Dec 7, 2021

View reviewed changes

components/dashboard/src/Login.tsx Outdated Show resolved Hide resolved

geropl reviewed Dec 7, 2021

View reviewed changes

jankeromnes reviewed Dec 7, 2021

View reviewed changes

components/dashboard/src/Login.tsx Outdated Show resolved Hide resolved

Adjust login page for prefix visit

21cdd69

laushinka force-pushed the laushinka/login-6826 branch from 5795c11 to 21cdd69 Compare December 7, 2021 11:48

laushinka added 2 commits December 7, 2021 11:51

Fix dark

8e8a984

Experiments fix

57b6fb2

geropl approved these changes Dec 7, 2021

View reviewed changes

roboquat assigned geropl Dec 7, 2021

roboquat added the lgtm label Dec 7, 2021

roboquat added the approved label Dec 7, 2021

roboquat merged commit 7b7193d into main Dec 7, 2021

roboquat deleted the laushinka/login-6826 branch December 7, 2021 12:50

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Dec 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Laushinka/login 6826 #7046

Laushinka/login 6826 #7046

laushinka commented Dec 2, 2021 •

edited

Loading

jankeromnes left a comment •

edited

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

roboquat commented Dec 3, 2021

laushinka commented Dec 3, 2021 •

edited

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jldec commented Dec 5, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jldec commented Dec 6, 2021

svenefftinge commented Dec 6, 2021

jankeromnes commented Dec 6, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 6, 2021

laushinka commented Dec 7, 2021

laushinka commented Dec 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Dec 7, 2021 •

edited

Loading

geropl Dec 7, 2021

jankeromnes Dec 7, 2021

geropl Dec 7, 2021 •

edited

Loading

geropl Dec 7, 2021

geropl Dec 7, 2021

geropl Dec 7, 2021

laushinka Dec 7, 2021

jankeromnes Dec 7, 2021 •

edited

Loading

laushinka Dec 7, 2021

geropl commented Dec 7, 2021

geropl left a comment

roboquat commented Dec 7, 2021

roboquat commented Dec 7, 2021

jldec commented Dec 12, 2021

Laushinka/login 6826 #7046

Laushinka/login 6826 #7046

Conversation

laushinka commented Dec 2, 2021 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

jankeromnes left a comment • edited Loading

Choose a reason for hiding this comment

laushinka commented Dec 3, 2021 • edited by werft-gitpod-dev-com bot Loading

jankeromnes commented Dec 3, 2021 • edited by werft-gitpod-dev-com bot Loading

laushinka commented Dec 3, 2021 • edited by werft-gitpod-dev-com bot Loading

roboquat commented Dec 3, 2021

laushinka commented Dec 3, 2021 • edited Loading

laushinka commented Dec 3, 2021 • edited by werft-gitpod-dev-com bot Loading

laushinka commented Dec 3, 2021 • edited by werft-gitpod-dev-com bot Loading

laushinka commented Dec 3, 2021 • edited by werft-gitpod-dev-com bot Loading

jldec commented Dec 5, 2021 • edited by werft-gitpod-dev-com bot Loading

jldec commented Dec 6, 2021

svenefftinge commented Dec 6, 2021

jankeromnes commented Dec 6, 2021 • edited by werft-gitpod-dev-com bot Loading

laushinka commented Dec 6, 2021

laushinka commented Dec 7, 2021

laushinka commented Dec 7, 2021 • edited by werft-gitpod-dev-com bot Loading

jankeromnes commented Dec 7, 2021 • edited Loading

geropl Dec 7, 2021

Choose a reason for hiding this comment

jankeromnes Dec 7, 2021

Choose a reason for hiding this comment

geropl Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

geropl Dec 7, 2021

Choose a reason for hiding this comment

geropl Dec 7, 2021

Choose a reason for hiding this comment

geropl Dec 7, 2021

Choose a reason for hiding this comment

laushinka Dec 7, 2021

Choose a reason for hiding this comment

jankeromnes Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

laushinka Dec 7, 2021

Choose a reason for hiding this comment

geropl commented Dec 7, 2021

geropl left a comment

Choose a reason for hiding this comment

roboquat commented Dec 7, 2021

roboquat commented Dec 7, 2021

jldec commented Dec 12, 2021

laushinka commented Dec 2, 2021 •

edited

Loading

jankeromnes left a comment •

edited

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 3, 2021 •

edited

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 3, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jldec commented Dec 5, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Dec 6, 2021 •

edited by werft-gitpod-dev-com bot

Loading

laushinka commented Dec 7, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Dec 7, 2021 •

edited

Loading

geropl Dec 7, 2021 •

edited

Loading

jankeromnes Dec 7, 2021 •

edited

Loading