-
Notifications
You must be signed in to change notification settings - Fork 239
feat(compass-collection): Fill in the script screen of the Generate Mock Data modal flow CLOUDP-333859 #7296
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
|
There is an existing patch(es) for this commit SHA: Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'. |
b85a359 to
9ef8bbd
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.
Looking good, left two comment on rendering and css.
...mpass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.spec.tsx
Outdated
Show resolved
Hide resolved
| {stepContent} | ||
| <div data-testid={`generate-mock-data-step-${currentStep}`} /> | ||
| <div data-testid={`generate-mock-data-step-${currentStep}`}> | ||
| {STEP_TO_STEP_CONTENT[currentStep]} |
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.
A bit of a nit, instead of instantiating these components on load, can we instead write this with a switch? Either in a function or a variable. That way we create them when we're rendering.
Here's an example in the code base:
| const formContent = useMemo(() => { |
If we want to keep the Record approach, using functions would also work:
const STEP_TO_STEP_CONTENT: Record<MockDataGeneratorStep, React.JSX.Element> = {
[MockDataGeneratorStep.SCHEMA_CONFIRMATION]: () => <SchemaConfirmationScreen />,
[MockDataGeneratorStep.SCHEMA_EDITOR]: () => <FakerSchemaEditor />,
[MockDataGeneratorStep.DOCUMENT_COUNT]: () => <></>, // TODO: Implement as part of CLOUDP-333856
[MockDataGeneratorStep.PREVIEW_DATA]: () => <></>, // TODO: Implement as part of CLOUDP-333857
[MockDataGeneratorStep.GENERATE_DATA]: () => <ScriptScreen />,
};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.
Got it. Went with the useMemo approach used in the linked code.
packages/compass-collection/src/components/mock-data-generator-modal/script-screen.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.
nice!
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! Wondering about the slight difference in how the password and the username are displayed (one placeholder is surrounded with quotes while the other is not). Asked designs here
Description
Types of changes