feat(compass-telemetry): Atlas Skills Banner CLOUDP-349749 #7441
Conversation
| import { | ||
| ExperimentTestGroup, | ||
| ExperimentTestName, | ||
| useAssignment, | ||
| useTelemetry, | ||
| } from '@mongodb-js/compass-telemetry/provider'; |
There was a problem hiding this comment.
compass-components are core / foundational components for compass and shouldn't have dependencies on anything very feature specific or tying it to external services like telemetry
There was a problem hiding this comment.
Updated and put component in telemetry since it's a component related to experimentation but open to moving it if theres somewhere better
There was a problem hiding this comment.
I think compass-components is an okay place for it, like the banner itself is a pretty normal React component. It will not always be an experiment, right? If experiment is a success and it stays, moving it around just because it's not an experiment anymore seems like a good indicator that we didn't put it in the appropriate place in the first place
There was a problem hiding this comment.
Put the component back in compass-components and kept the hook w/ experiment logic in compass-telemetry
There was a problem hiding this comment.
Looks like a good split! Then later down the road we just remove the hook and the component stays as-is
| import Badge, { Variant as BadgeVariant } from '@leafygreen-ui/badge'; | ||
| import Icon from '@leafygreen-ui/icon'; | ||
| // @ts-expect-error - Using icon v14 via alias for Award icon | ||
| import AwardIcon from '@leafygreen-ui/icon-v14/dist/Award'; |
There was a problem hiding this comment.
This is somewhat a solution, but we're currently already not in a great shape in terms of our leafygreen dependencies due to how complex the update to latest turning out to be and I'd rather we don't add to this problem. I think there are two ways forward here:
- Just inline the icon in this file as an inline svg component. This is honestly the most straightforward and the "cleanest" one for the time being while we're dealing with the leafygreen update
- Try to update icon package only to 14. As we learned leafygreen major doesn't always mean functional breaking change, so it might be possible to update the icons package without updating everything else here. It's a bit involved though, so might not really go very easy. I probably can try to do this bump for you and if it's not working out, I'd suggest we go with my first suggestion here
There was a problem hiding this comment.
Seems like bumping icon package went smootly, just need to get a review on #7450 before merging, but after that you should be able to use this new icon you need
There was a problem hiding this comment.
Nice! I'll leave in draft and then update when merged
carolynmcca
force-pushed
the
CLOUDP-349749
branch
from
3310f21 to
3bbfc67
Compare
carolynmcca
changed the title
carolynmcca
changed the title
| * additionalProperties: { screen: 'aggregations' }, | ||
| * }); | ||
| */ | ||
| export const useFireExperimentViewed = ({ |
There was a problem hiding this comment.
I'm not exactly sure why the other growth team didn't do this, but seems like in addition to other experiment helpers that are being passed from mms to compass through experimentation provider, the useTrackIsInSample hook also should be passed down and used for tracking instead of this custom tracking function. Feels like it's worth changing, otherwise we are only like half way integrated with experiments sdk properly. Do you mind including this change in your scope?
There was a problem hiding this comment.
The experiment utils we now pass into compass have parameters to call trackIsInSample built in. (This is generally the more idiomatic approach. useFireExperimentViewed is a remnant of when we didn't pass in the experiment utils.) We would only need the separate hook if we need to call experiment viewed at a different place from either assigning or retrieving experiment assignments. If this is the case here, then yes we should pass useTrackIsInSample as well.
carolynmcca
force-pushed
the
CLOUDP-349749
branch
from
9a29906 to
5ac62c2
Compare
| ".": "./src/index.ts", | ||
| "./provider": "./src/provider.tsx" | ||
| "./provider": "./src/provider.tsx", | ||
| "./atlas-skills": "./src/atlas-skills.ts" |
There was a problem hiding this comment.
Super nit: just for the ease of cleanup later, you can probably just re-export through provider path, but having a dedicated named export also works
carolynmcca
force-pushed
the
CLOUDP-349749
branch
from
fa41176 to
72eb33f
Compare
carolynmcca
requested review from
gribnoysup and
paula-stacho
and removed request for
paula-stacho
| ExperimentTestGroup.atlasSkillsVariant; | ||
|
|
||
| // Track experiment viewed when user is in experiment and banner would be shown | ||
| useTrackInSample(ExperimentTestName.atlasSkills, !!atlasSkillsAssignment, { |
There was a problem hiding this comment.
Generally we also track is in sample when the user is in the control as well. Is there a reason you're limiting to the variant here?
There was a problem hiding this comment.
It'll fire if they're in variant or control - it won't fire if atlasSkillsAssignment is null or undefined
I'll update comment since it's confusing
There was a problem hiding this comment.
Yeah, and banner would be shown is incorrect then right?
There was a problem hiding this comment.
yeah updated comment!
packages/compass-components/src/components/atlas-skills-banner.tsx
Outdated
Show resolved
Hide resolved
4 participants
Description
Creating reusable banner that will be used for atlas skills experiment documented in https://jira.mongodb.org/browse/CLOUDP-346311; see below for screenshot of example banner. In order to use new award icon added an alias for new icon package version since it'd be a major change to update all dependent packages.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes