Jon/ab/create-login-buttons #4473
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jon-edge
force-pushed
the
jon/ab/create-login-buttons
branch
3 times, most recently
from
45ebbfc
to
3603a69
Compare
paullinator
requested changes
Sep 22, 2023
src/experimentConfig.ts
Outdated
| } catch (err) { | ||
| // Not found or incompatible. Re-generate with random values according to | ||
| // the defined distribution. | ||
| const generatedExperimentConfig = asExperimentConfig({}) |
There was a problem hiding this comment.
Consolidate isMaestro() check here and return default values if so
src/components/scenes/LoginScene.tsx
Outdated
| }, []) | ||
|
|
||
| return loggedIn ? ( | ||
| return loggedIn || (!isMaestro() && experimentConfig == null) ? ( |
There was a problem hiding this comment.
consolidate ismaestro checks to getExperimentConfig
| const generateExperimentConfigVal = (key: keyof typeof experimentDistribution): boolean => { | ||
| return Math.random() < experimentDistribution[key] | ||
| const generateExperimentConfigVal = <T>(key: keyof typeof experimentDistribution, configVals: T[]): T => { | ||
| const variantProbability = experimentDistribution[key] |
There was a problem hiding this comment.
Should assert that variantProbability.length === configVals.length - 1
Jon-edge
force-pushed
the
jon/ab/create-login-buttons
branch
2 times, most recently
from
8fd3040
to
df5d072
Compare
paullinator
requested changes
Sep 25, 2023
src/experimentConfig.ts
Outdated
| }) | ||
| // behavior/appearance. | ||
| // If no generateCfgValFn given, returns defined defaults | ||
| const asExperimentConfig: any = (generateCfgValFn?: GenerateExperimentConfigValFn) => |
There was a problem hiding this comment.
A cleaner shouldn't be typed any. This would break the return type
src/experimentConfig.ts
Outdated
| @@ -74,7 +83,7 @@ const experimentConfigPromise: Promise<ExperimentConfig> = (async (): Promise<Ex | |||
| } catch (err) { | |||
| // Not found or incompatible. Re-generate with random values according to | |||
| // the defined distribution. | |||
| const generatedExperimentConfig = asExperimentConfig({}) | |||
| const generatedExperimentConfig = asExperimentConfig(generateExperimentConfigVal)({}) | |||
There was a problem hiding this comment.
Why all the complexity of creating a function that gets passed in when all you really care about is a boolean like useDefaults. Pass that as the first arg to asExperimentConfig and you don't need to check for a null function.
Jon-edge
force-pushed
the
jon/ab/create-login-buttons
branch
2 times, most recently
from
91b3e4d
to
df584ad
Compare
paullinator
approved these changes
Sep 26, 2023
Jon-edge
force-pushed
the
jon/ab/create-login-buttons
branch
from
df584ad
to
a63ac39
Compare
It's OK that we change the values of legacyLanding because it's not reporting values properly to the reports currently.
Jon-edge
force-pushed
the
jon/ab/create-login-buttons
branch
from
a63ac39
to
2ac7ae4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGELOG
Dependencies
#4469
Requirements
If you have made any visual changes to the GUI. Make sure you have: