-
Notifications
You must be signed in to change notification settings - Fork 581
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
feat: implement activity dot indicator experiment #11319
Conversation
7e0134c
to
7ce2bb9
Compare
And define a payload that allows us to force the dots to display (for QA)
Strangely, I don't see how the profile icon's dot ever gets requested so I've added the ability to force it here
3b6fcb9
to
3114cb9
Compare
3114cb9
to
ad75562
Compare
fallbackEnabled: true, | ||
fallbackVariant: "control", | ||
variantSuggestions: ["control", "variant-b", "variant-c"], | ||
payloadSuggestions: ['{"forceDots": "true"}', '{"forceDots": "false"}'], |
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.
Hmmm is is okay to have 3 variant and 2 payload suggestions?
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.
Yep, I believe the payload is just "additional" information that can be used in the rendering logic. That's what I'm utilizing to force the display of the activity dots for QA purposes.
// Dev Menu helper to force visible dots for testing during QA | ||
const forceDots = Boolean(payload && JSON.parse(payload)?.forceDots === "true") | ||
|
||
const color: Color = enabled ? (variant === "variant-b" ? "red50" : "blue100") : "blue100" |
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 think for this experiment it is fine as it doesn't touch a lot of functionality, but if it was a bigger experiment we could also use a feature flag just to make sure if something goes wrong with the implementation of the experiment we can always fall back.
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.
The Enabled toggle in Unleash effectively gives us that ability, no? That's why this logic on L14 accounts for enabled
.
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.
Yes, if the implementation of the experiment is correct. For small experiments it should be fine not to use a ff.
I just wanted to spread the knowledge @olerichter00 shared with me while working on the Artwork card redesign - for that experiment it was recommended to use the feature flag because the experiment's logic was more complex and used in a lot of components. So if there was a mistake that affects the experiment's results we would disable the experiment and the feature flag, fix the issue and enable the ff and the experiment again
I hope it makes sense 😅
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.
An additional Echo feature flag can be useful when implementing a feature that takes a long time across multiple app releases. In this case, enabling the flag in Unleash would also activate the feature for app versions that only have a work-in-progress state.
If the feature is ready within a single release cycle, the extra flag is unnecessary.
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.
Whoops I wrote, but forgot to submit, these comments before requesting review 🙈
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.
This hook encapsulates the color calculation and other experiment-related features to simplify the invocation of the experiment at the various call sites.
variantSuggestions: ["control", "variant-b", "variant-c"], | ||
payloadSuggestions: ['{"forceDots": "true"}', '{"forceDots": "false"}'], |
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.
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.
Thanks!
const { trackExperiment: trackActvityDotExperiment } = useActivityDotExperiment() | ||
|
||
useEffect(() => { | ||
trackActvityDotExperiment() | ||
}, []) |
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.
We fire the experiment_viewed
event once per session this way.
case "profile": { | ||
if (forceDots) { | ||
tabsBadges[tab] = { | ||
tabBarBadge: unreadConversationsCount, | ||
tabBarBadge: "", | ||
tabBarBadgeStyle: { | ||
backgroundColor: color("red100"), | ||
...visualClueStyles, | ||
}, | ||
} | ||
} |
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.
The profile
branch of this switch statement was missing, and thus the blue profile dot is currently unable to be displayed.
After discussing with @gkartalis I think that may have been a deliberate decision, in which case we can drop this case "profile"
branch as well. But in the meantime this allows us to force the dot to display during QA.
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.
@MounirDhahri this^ was our finding, in case you have any further insight on this — is the bottom nav profile icon expected to display a dot after all?
- Use new var instead of mutating destructured var from props - Avoid 'px' in positioning props
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.
Thanks for addressing the comments, looks good!
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.
Awesome! thanks!
variantSuggestions: ["control", "variant-b", "variant-c"], | ||
payloadSuggestions: ['{"forceDots": "true"}', '{"forceDots": "false"}'], |
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.
Thanks!
This PR resolves ONYX-1465
Description
Sets up the activity dot experiment
TODO:
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.