-
Notifications
You must be signed in to change notification settings - Fork 245
feat: setting up early indexes experimentation and experiment viewed event CLOUDP-311776 #6860
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
Changes from 14 commits
61384ae
4a08032
fda1d08
1c42217
2b288bb
22fd8f6
b10c782
a3990c3
da566de
18168b9
07c902d
31b419d
5fa1a06
8001399
349d10e
0307155
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import React from 'react'; | ||
| import { render, screen } from '@mongodb-js/testing-library-compass'; | ||
| import CreateIndexModalHeader from './create-index-modal-header'; | ||
| import { expect } from 'chai'; | ||
|
|
||
| describe('CreateIndexModalHeader', () => { | ||
| it('renders the modal title', () => { | ||
| render(<CreateIndexModalHeader />); | ||
| const title = screen.getByTestId('create-index-modal-header-title'); | ||
|
|
||
| expect(title.textContent).to.be.equal('Create Index'); | ||
| }); | ||
|
|
||
| it('renders the subtitle text', () => { | ||
| render(<CreateIndexModalHeader />); | ||
| const subtitle = screen.getByTestId('create-index-modal-header-subtitle'); | ||
| expect(subtitle).to.exist; | ||
| }); | ||
|
|
||
| it('renders the link to the Index Strategies Documentation', () => { | ||
| render(<CreateIndexModalHeader />); | ||
| const link = screen.getByRole('link', { | ||
| name: /Index Strategies Documentation/i, | ||
| }); | ||
| expect(link).to.exist; | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,42 @@ | ||||||
| import { | ||||||
| H3, | ||||||
| Body, | ||||||
| spacing, | ||||||
| css, | ||||||
| cx, | ||||||
| palette, | ||||||
| Link, | ||||||
| } from '@mongodb-js/compass-components'; | ||||||
| import React from 'react'; | ||||||
|
|
||||||
| const headerStyle = css({ | ||||||
| padding: spacing[5], | ||||||
|
||||||
| padding: spacing[5], | |
| padding: spacing[800], |
nit The single digit values here are deprecated (we'll be updating all of them in #6871 )
Outdated
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.
| <div className={cx(headerStyle)}> | |
| <div className={headerStyle}> |
Outdated
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.
Small nit, we typically avoid using style unless it's a dynamic value we're passing to it. Instead we would create a css style variable and pass that to the className like we do for headerStyle.
Outdated
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.
Should we account for dark mode here? Is there a reason we need to explicitly set the color here and not use the default color? Typically we try to default to the colors from the leafygreen typography components. They're also plugged into the dark mode provider so they automatically switch when it changes.
If we explicitly want to have a color here, and we want it to change based on the theme, you can look for the useDarkMode() hook in the code base as a reference.
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.
designs for dark mode will come out but is not available right now, here's a ticket: https://jira.mongodb.org/browse/CLOUDP-313986.
i only followed the design for the dark gray color though and in the design it was that color
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -21,8 +21,12 @@ import type { RootState } from '../../modules'; | |||||
| import { | ||||||
| useTrackOnChange, | ||||||
| type TrackFunction, | ||||||
| useFireExperimentViewed, | ||||||
| TestName, | ||||||
| } from '@mongodb-js/compass-telemetry/provider'; | ||||||
| import { useConnectionInfoRef } from '@mongodb-js/compass-connections/provider'; | ||||||
| import { usePreference } from 'compass-preferences-model/provider'; | ||||||
| import CreateIndexModalHeader from './create-index-modal-header'; | ||||||
|
|
||||||
| type CreateIndexModalProps = React.ComponentProps<typeof CreateIndexForm> & { | ||||||
| isVisible: boolean; | ||||||
|
|
@@ -70,13 +74,28 @@ function CreateIndexModal({ | |||||
| undefined | ||||||
| ); | ||||||
|
|
||||||
| // @experiment Early Journey Indexes Guidance & Awareness | Jira Epic: CLOUDP-239367 | ||||||
| const enableInIndexesGuidanceExp = usePreference('enableIndexesGuidanceExp'); | ||||||
| const showIndexesGuidanceVariant = | ||||||
| usePreference('showIndexesGuidanceVariant') && enableInIndexesGuidanceExp; | ||||||
|
|
||||||
| useFireExperimentViewed({ | ||||||
| testName: TestName.earlyJourneyIndexesGuidance, | ||||||
| shouldFire: enableInIndexesGuidanceExp && isVisible, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'm understanding this wrong, should this be
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope this should fire for when user is in the control as well so it should be
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we track that they're in the control then? Or is there already a way we differentiate that they are shown/not shown the new experience?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this tracks if they are in the control as well, if they are in the experiment
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make things easier if we track if they're in the control or shown the experience here as some metadata for the tracking event? If it's on the mms side I suppose we don't need to since we'll be able to correlate them anyway. Feel free to resolve! |
||||||
| }); | ||||||
|
|
||||||
| return ( | ||||||
| <Modal | ||||||
| open={isVisible} | ||||||
| setOpen={onSetOpen} | ||||||
| data-testid="create-index-modal" | ||||||
| size={showIndexesGuidanceVariant ? 'large' : 'default'} | ||||||
| > | ||||||
| <ModalHeader title="Create Index" subtitle={namespace} /> | ||||||
| {showIndexesGuidanceVariant ? ( | ||||||
| <CreateIndexModalHeader /> | ||||||
| ) : ( | ||||||
| <ModalHeader title="Create Index" subtitle={namespace} /> | ||||||
| )} | ||||||
|
|
||||||
| <ModalBody> | ||||||
| <CreateIndexForm namespace={namespace} {...props} /> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export enum TestName { | ||
| earlyJourneyIndexesGuidance = 'EARLY_JOURNEY_INDEXES_GUIDANCE_20250328', | ||
| } |
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're already exporting these from the
leafygreen.tsxfile:compass/packages/compass-components/src/components/leafygreen.tsx
Line 162 in 7371e1b
So we can remove this.