-
Notifications
You must be signed in to change notification settings - Fork 229
feat(compass-collection): Add mock data generator modal container CLOUD-333848 #7156
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
Conversation
packages/compass-collection/src/components/mock-data-generator-modal/mock-generator-modal.tsx
Outdated
Show resolved
Hide resolved
packages/compass-collection/src/components/mock-data-generator-modal/mock-generator-modal.tsx
Outdated
Show resolved
Hide resolved
packages/compass-collection/src/components/mock-data-generator-modal/types.ts
Outdated
Show resolved
Hide resolved
...es/compass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.tsx
Outdated
Show resolved
Hide resolved
...mpass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.spec.tsx
Outdated
Show resolved
Hide resolved
7824ec1
to
341cdf8
Compare
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 introduces a mock data generator modal component to the compass-collection package as part of feature CLOUD-333848. The modal provides a multi-step wizard interface for generating mock data with AI integration.
- Adds a new modal component with step-based navigation and proper state management
- Implements TypeScript types and constants for the mock data generation workflow
- Includes comprehensive test coverage for modal behavior and interactions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
types.ts |
Defines the MockDataGeneratorStep enum with 6 workflow steps |
mock-data-generator-modal.tsx |
Main modal component with navigation logic and step management |
mock-data-generator-modal.spec.tsx |
Test suite covering modal rendering, navigation, and user interactions |
constants.ts |
Maps step enum values to corresponding button labels |
Comments suppressed due to low confidence (1)
packages/compass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.spec.tsx:91
- This test iterates through all button labels but doesn't verify that the modal actually progresses through the correct steps. The test should verify that clicking the button advances to the expected next step, not just check the label text.
Object.values(StepButtonLabelMap).forEach((label) => {
...mpass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.spec.tsx
Outdated
Show resolved
Hide resolved
).to.exist; | ||
}); | ||
|
||
it('calls setIsOpen(false) when the modal is closed', () => { |
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.
Nice test coverage!
...mpass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.spec.tsx
Outdated
Show resolved
Hide resolved
...mpass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.spec.tsx
Outdated
Show resolved
Hide resolved
|
||
const rightButtonsStyles = css` | ||
display: flex; | ||
gap: 8px; |
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 use LG's spacing
consts (import via @mongodb-js/compass-components
)
isOpen: boolean; | ||
setIsOpen: (isOpen: boolean) => void; | ||
currentStep: MockDataGeneratorStep; | ||
setCurrentStep: (step: MockDataGeneratorStep) => void; |
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.
why isn't the currentStep state managed by this component?
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.
See this comment
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.
Essentially, modal isn't always initialized to same screen. An alternative I just thought of could be to pass in the initial current step as a prop, and still leave this component to manage the state. That's probably the cleanest actually
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.
Nice work Nataly! Approved, left one small nit we could consider (perhaps fixing in next ticket even)
isOpen: boolean; | ||
setIsOpen: (isOpen: boolean) => void; | ||
currentStep: MockDataGeneratorStep; | ||
setCurrentStep: (step: MockDataGeneratorStep) => void; |
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 following React convention for callback prop naming
isOpen: boolean; | |
setIsOpen: (isOpen: boolean) => void; | |
currentStep: MockDataGeneratorStep; | |
setCurrentStep: (step: MockDataGeneratorStep) => void; | |
isOpen: boolean; | |
onOpenChange: (isOpen: boolean) => void; | |
currentStep: MockDataGeneratorStep; | |
onCurrentStepChange: (step: MockDataGeneratorStep) => void; |
2fa272e
to
e36b798
Compare
onOpenChange(false); | ||
resetState(); |
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.
Resetting the state like that means that the modal content jumps around while it animates from the screen, for that purpose in compass we usually reset the state when opening the modal, not closing, this is a way nicer visual experience usually, especially if your steps are very different visually
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 what Sergey's saying is we can just take out those resetState
calls, since we will be passing in currentStep
anyway
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'm saying you want to reset it on open, not on close, if you don't want UI to feel janky. I am not saying you don't want to reset the state at all. Even if you're passing currentStep
from some other place (redux store or parent component), something would still need to reset the state at some point
@@ -18,51 +23,51 @@ const footerStyles = css` | |||
|
|||
const rightButtonsStyles = css` | |||
display: flex; | |||
gap: 8px; | |||
gap: ${spacing[200]}px; |
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.
Q: So this corresponds to the 8px from before?
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.
LGTM!
}: Props) => { | ||
const onNext = () => { | ||
if (currentStep < MockDataGeneratorStep.GENERATE_DATA) { | ||
onCurrentStepChange(currentStep + 1); |
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 code is brittle as it allows to pass an arbitrary number value here, the code below is completely valid from TypeScript perspective:
onCurrentStepChange(currentStep + 42);
If you are opting into using enums (we generally avoid them in the codebase, using string unions instead, even string enum would be a better choice) to introduce rigid typings for allowed steps, I would suggest that the step transitions are clearly defined through switch case statements to disallow such behavior (you can extract this logic into its own method that can be shared later with other parts of the code or easily moved around). I would also strongly recommend to switch to either string enums or string unions to make the transitions clearer and avoid the pitfals of using numbered enums
Description
Jira Ticket: https://jira.mongodb.org/browse/CLOUDP-333848
Adds
MockDataGeneratorModal
in thecompass-collection
package.Checklist
Motivation and Context
Open Questions
Dependents
Types of changes