Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { MockDataGeneratorStep } from './types';

export const StepButtonLabelMap = {
[MockDataGeneratorStep.AI_DISCLAIMER]: 'Use Natural Language',
[MockDataGeneratorStep.SCHEMA_CONFIRMATION]: 'Confirm',
[MockDataGeneratorStep.SCHEMA_EDITOR]: 'Next',
[MockDataGeneratorStep.DOCUMENT_COUNT]: 'Next',
[MockDataGeneratorStep.PREVIEW_DATA]: 'Generate Script',
[MockDataGeneratorStep.GENERATE_DATA]: 'Done',
} as const;
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { expect } from 'chai';
import React from 'react';
import { render, screen } from '@mongodb-js/testing-library-compass';
import Sinon from 'sinon';
import MockDataGeneratorModal from './mock-data-generator-modal';
import { MockDataGeneratorStep } from './types';
import { StepButtonLabelMap } from './constants';

describe('MockDataGeneratorModal', () => {
const sandbox = Sinon.createSandbox();
let onOpenChange: Sinon.SinonSpy;

beforeEach(() => {
onOpenChange = sandbox.spy();
});

afterEach(() => {
sandbox.restore();
});

function renderModal({
isOpen = true,
currentStep = MockDataGeneratorStep.AI_DISCLAIMER,
} = {}) {
function MockDataGeneratorModalWrapper() {
const [currentStepStateMock, onCurrentStepChangeStateMock] =
React.useState<MockDataGeneratorStep>(currentStep);
return (
<MockDataGeneratorModal
isOpen={isOpen}
onOpenChange={onOpenChange}
currentStep={currentStepStateMock}
onCurrentStepChange={(step) => {
onCurrentStepChangeStateMock(step);
}}
/>
);
}
return render(<MockDataGeneratorModalWrapper />);
}

it('renders the modal when isOpen is true', () => {
renderModal();

expect(screen.getByTestId('generate-mock-data-modal')).to.exist;
});

it('does not render the modal when isOpen is false', () => {
renderModal({ isOpen: false });

expect(screen.queryByTestId('generate-mock-data-modal')).to.not.exist;
});

it('calls onOpenChange(false) when the modal is closed', () => {
renderModal();

screen.getByLabelText('Close modal').click();

expect(onOpenChange.calledOnceWith(false)).to.be.true;
});

it('calls onOpenChange(false) when the cancel button is clicked', () => {
renderModal();

screen.getByText('Cancel').click();

expect(onOpenChange.calledOnceWith(false)).to.be.true;
});

it('disables the Back button on the first step', () => {
renderModal();

expect(
screen.getByRole('button', { name: 'Back' }).getAttribute('aria-disabled')
).to.equal('true');
});

describe('when rendering the modal in a specific step', () => {
const steps = Object.keys(
StepButtonLabelMap
) as unknown as MockDataGeneratorStep[];

steps.forEach((currentStep) => {
it(`renders the button with the correct label when the user is in step "${currentStep}"`, () => {
renderModal({ currentStep });
expect(screen.getByTestId('next-step-button')).to.have.text(
StepButtonLabelMap[currentStep]
);
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import React from 'react';

import {
css,
ModalBody,
ModalHeader,
spacing,
} from '@mongodb-js/compass-components';

import {
Button,
Modal,
ModalFooter,
ButtonVariant,
} from '@mongodb-js/compass-components';
import { MockDataGeneratorStep } from './types';
import { StepButtonLabelMap } from './constants';

const footerStyles = css`
flex-direction: row;
justify-content: space-between;
`;

const rightButtonsStyles = css`
display: flex;
gap: ${spacing[200]}px;
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flex-direction: row;
`;

interface Props {
isOpen: boolean;
onOpenChange: (isOpen: boolean) => void;
currentStep: MockDataGeneratorStep;
onCurrentStepChange: (step: MockDataGeneratorStep) => void;
}

const MockDataGeneratorModal = ({
isOpen,
onOpenChange,
currentStep,
onCurrentStepChange,
}: Props) => {
const resetState = () => {
onCurrentStepChange(MockDataGeneratorStep.AI_DISCLAIMER);
};

const onNext = () => {
if (currentStep < MockDataGeneratorStep.GENERATE_DATA) {
onCurrentStepChange(currentStep + 1);
Copy link
Collaborator

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

} else {
onOpenChange(false);
resetState();
}
};

const onBack = () => {
if (currentStep > MockDataGeneratorStep.AI_DISCLAIMER) {
onCurrentStepChange(currentStep - 1);
}
};

const onCancel = () => {
onOpenChange(false);
resetState();
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator

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

};

return (
<Modal
open={isOpen}
setOpen={(open) => onOpenChange(open)}
data-testid="generate-mock-data-modal"
>
<ModalHeader title="Generate Mock Data" />
<ModalBody>
{/* TODO: Render actual step content here based on currentStep. (CLOUDP-333851) */}
<div data-testid={`generate-mock-data-step-${currentStep}`} />
</ModalBody>
<ModalFooter className={footerStyles}>
<Button
onClick={onBack}
disabled={currentStep === MockDataGeneratorStep.AI_DISCLAIMER}
>
Back
</Button>
<div className={rightButtonsStyles}>
<Button onClick={onCancel}>Cancel</Button>
<Button
variant={ButtonVariant.Primary}
onClick={onNext}
data-testid="next-step-button"
>
{StepButtonLabelMap[currentStep]}
</Button>
</div>
</ModalFooter>
</Modal>
);
};

export default MockDataGeneratorModal;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export enum MockDataGeneratorStep {
AI_DISCLAIMER = 1,
SCHEMA_CONFIRMATION = 2,
SCHEMA_EDITOR = 3,
DOCUMENT_COUNT = 4,
PREVIEW_DATA = 5,
GENERATE_DATA = 6,
}
Loading