-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove experiments from Login page #7349
Conversation
@@ -28,7 +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, // https://github.com/gitpod-io/gitpod/issues/6826 | |||
"login-from-context-6826": 0, // https://github.com/gitpod-io/gitpod/issues/6826 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about commenting out this line but leaving the value at 0.5?
This way
-
(I think) new users should no longer see this particular experiment in their Localstorage - we don't want old experiments to keep accumulating.
-
The comment preserves the value used for the experiment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - I tried commenting out line 31 in 8363c65, but that produced a typescript error
so if we did move forward with this suggestion, that would need to be fixed too.
/tmp/build/components-dashboard--app.d40dd25a6d6f22b76eea40734e5961a545a272a7/src/experiments.ts
TypeScript error in /tmp/build/components-dashboard--app.d40dd25a6d6f22b76eea40734e5961a545a272a7/src/experiments.ts(47,17):
Type 'boolean' is not assignable to type 'never'. TS2322
45 | for (const experiment of Object.keys(Experiments) as Experiment[]) {
46 | if (!(experiment in result)) {
> 47 | result[experiment] = Math.random() < Experiments[experiment];
| ^
48 | }
49 | }
50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @geropl for review while @laushinka is still out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried commenting out line 31
@jldec Yes, that's unfortunate, and rooted in my missing TS-typings skills ;-) IMO it's better to remove it (to have the compiler ensures it's not used anymore) and uncomment the example
thing that's still in there.
Will apply that in top here, and approve afterwards if there are no objections.
/lgtm |
LGTM label has been added. Git tree hash: 4c0817557267de4bc10486044d0b4d10982f1b42
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geropl Associated issue: #7329 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:
Approvers can indicate their approval by writing |
Description
Removes the experiment from the Login page.
Related Issue(s)
Fixes #7329
How to test
Append a Git repo link to
https://laushinka-remove-ui-experiment-7329.staging.gitpod-dev.com/#
Release Notes
Documentation