-
Notifications
You must be signed in to change notification settings - Fork 229
feat(compass-collection): Add Disclaimer Screen - Mock Data Generator CLOUDP-333851 #7212
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
base: main
Are you sure you want to change the base?
Conversation
packages/compass-generative-ai/src/store/atlas-optin-reducer.ts
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
This PR updates the AI opt-in modal design and integrates it with the mock data generator feature. The main purpose is to show an AI disclaimer screen before allowing users to access the mock data generator, replacing the previous AI disclaimer step with a new schema confirmation step as the initial modal state.
Key changes include:
- Updated AI opt-in modal to use MarketingModal component with new design
- Added AppRegistry event emission after AI opt-in to trigger mock data generator modal
- Modified mock data generator to start at schema confirmation step instead of AI disclaimer
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/compass-telemetry/src/provider.tsx | Added getAssignment method to experimentation services interface |
packages/compass-telemetry/src/experimentation-provider.tsx | Added GetAssignmentFn type and getAssignment prop to context provider |
packages/compass-generative-ai/src/store/atlas-optin-reducer.ts | Added localAppRegistry to thunk actions and emit event after opt-in success |
packages/compass-generative-ai/src/components/ai-optin-modal.tsx | Updated modal to use MarketingModal component with new design |
packages/compass-generative-ai/src/components/ai-image-banner.tsx | Updated SVG banner dimensions and graphics |
packages/compass-collection/src/stores/collection-tab.ts | Added event listener for mock data generator modal opening |
packages/compass-collection/src/modules/collection-tab.ts | Changed initial mock data generator step and added modal opening logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/compass-generative-ai/src/components/ai-optin-modal.tsx
Outdated
Show resolved
Hide resolved
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: The changes here for the opt-in model are the same as the changes that were aiming to be made for #7164
This is more up to date with the designs so I think I'll close that PR and we can use the changes one to resolve the tickets tied to them.
<ConfirmationModal | ||
<MarketingModal | ||
className={css({ | ||
zIndex: '99999', |
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 zIndex implies there's a visual conflict with another modal.
Slightly better: zIndex: String(otherZIndex + 10) to make that relationship clearer
Better: No two modals render at the same time
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.
Removed as we no longer seem to need to modify zIndex with the v8.0 marketing modal
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.
Makes sense to me from a line-by-line POV. Will defer to another if the diffs are okay for the application architecture
packages/compass-generative-ai/src/store/atlas-optin-reducer.ts
Outdated
Show resolved
Hide resolved
packages/compass-generative-ai/src/store/atlas-optin-reducer.ts
Outdated
Show resolved
Hide resolved
@@ -104,25 +81,34 @@ export const AIOptInModal: React.FunctionComponent<OptInModalProps> = ({ | |||
}, [track, onOptInModalClose]); | |||
|
|||
return ( | |||
<ConfirmationModal | |||
<MarketingModal | |||
className={css({ |
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.
Don't use css
in render method, this creates a new class name every time component updates and is not performant, static styles can be defined with css
outside of render, dynamic should be passed through the style prop. But also this probably indicates that something is not right, we shouldn't have two modals on the screen at the same time, so can you clarify why this is happening?
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.
static styles can be defined with css outside of render
This makes sense!
we shouldn't have two modals on the screen at the same time, so can you clarify why this is happening?
Without the zIndex
, the modal gets cut off by the Atlas nav/header. It probably doesn't need to be set to such a high value though.
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.
Removed as we no longer seem to need to modify zIndex with the v8.0 marketing modal
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.
@ncarbon gotca. We added withStackedComponentStyles
some time ago to resolve the issue with overlapping styles, seems like marketing modal was just not wrapped in it for some reason, I think wrapping it before exporting from compass-components, similar to other modals should resolve your issue
// TODO: Re-enable this test once the LG-5416 is released | ||
it.skip('should show the opt in button enabled when project AI setting is enabled', async function () { |
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 seems too important to just skip. If we can't disable the button, we should make sure that clicking it doesn't cause a request to opt-in and test that instead, not just skip the test
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.
Un-skipped and using v8.0 Marketing Modal
…/mock-data-gen-disclaimer
palette, | ||
} from '@mongodb-js/compass-components'; | ||
// eslint-disable-next-line @mongodb-js/compass/no-leafygreen-outside-compass-components |
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.
Importing MarketingModal
directly from LeafyGreen instead of going through compass-components because we need v7+ to disable the button. The compass-components package is v5.0.2, and upgrading MarketingModal in compass-components causes a bunch of cascading dependency upgrades that are better dealt with in the separate ticket that Compass team is working on to upgrade the modal: https://jira.mongodb.org/browse/COMPASS-9595. I've added a comment to that ticket to switch this import as well
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 can't do this, we have all these components in one place for a reason, having multiple versions of the same leafygreen component causes hard to predict issues across the whole application
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.
Yeah this is why that ticket was blocked but this may be an acceptable enough temporary workaround. We're usually strict about going through compass-components
though so not sure. Alternative is to style / disable buttons through CSS hacks which isn't much better IMO
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.
Makes sense! Will go with the disabled button style CSS hack
@gagik I think we should keep your Pull Request separate. This PR is fairly large already, and the upgrade to MarketingModal in the shared component package is somewhat involved with all the other dependency upgrades it necessitates. |
@jcobis My PR was a subset of changes here so there isn't much else to follow up (the designs in it are old). So far I don't see any further changes needed. |
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.
Please remove the override for the no-leafygreen-outside-compass-components rule and remove the dependency from the package, this is not how we do things in compass, those rules are set up for a reason: having multiple misaligned versions of leafygreen bundled in the same application causes hard to track and predict issues sometimes making the app unusable
One alternative could be to copy the changes in https://github.com/mongodb-js/compass/pull/7164/files/a6e2b755d5654643c5534665e212f289c8d29e26#diff-92bc76f52506e710975854f3f55a5e71b31ccf2fb807f36e98461d85ba226edaR39 (perhaps with styles Anna linked) to make the button look disabled. Alternatively if we don't want to mess with styling and just make the button no-op. |
…/mock-data-gen-disclaimer
Let's definitely go with the styling workaround. We can't have the only CTA button to proceed enabled and clickable but dead with no conveyed reason why, that would feel like an error state and block/confuse users. |
Description
AIOptinModal
to match the new design changes (Figma).openMockDataGeneratorModal
to showAIOptinModal
if the user hasn't opted into AI features.AppRegistry
to emit "open-mock-data-generator-modal" after the user opts into AI features via the Optin modal.compass-collection
introduced a cyclical dependency.Checklist
Motivation and Context
To show an AI disclaimer screen before showing the mock data generator feature UI.
Types of changes