PORTALS-2624: remove bootstrap-4-backport class from CreateProjectModal #158
Conversation
…al, convert to Mui Dialog
hallieswan
force-pushed
the
PORTALS-2624_CreateProjectModal
branch
from
77eadf6 to
9f1aa5a
Compare
hallieswan
force-pushed
the
PORTALS-2624_CreateProjectModal
branch
from
9f1aa5a to
cdb0407
Compare
| inputProps={{ | ||
| onKeyDown: (event: React.KeyboardEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
TextField doesn't expose onKeyDown as a prop, so pass to Input via inputProps, see here
There was a problem hiding this comment.
Makes sense. Ideally this would all be wrapped in a form and confirm button would trigger the form submission event so we don't have to add this custom handler. I'm guessing that was originally difficult to do with a Bootstrap modal and probably equally difficult in a MUI Dialog.
| content={dialogContent} | ||
| confirmButtonText="Save" | ||
| onConfirm={onCreateProject} | ||
| onCancel={hide} |
There was a problem hiding this comment.
Previously, clicking the modal close button called hide, which cleared the alert and text entered into the input before calling onClose whereas clicking "Cancel" only called onClose.
Now, ConfirmationDialog will call hide both when clicking "Cancel" or when clicking the modal close button.
| it('Creates project on enter', async () => { | ||
| const { user, input, saveButton } = setUp() | ||
|
|
||
| await user.type(input, MOCK_PROJECT_NAME) | ||
| expect(input.value).toBe(MOCK_PROJECT_NAME) | ||
| await user.keyboard('{ENTER}') |
There was a problem hiding this comment.
Separate test for pressing enter rather than clicking "Save" button
| it('Clears alert and project name on cancel', async () => { | ||
| const { user, input, saveButton } = setUp() | ||
|
|
||
| await user.type(input, MOCK_INVALID_PROJECT_NAME) | ||
| await user.click(saveButton) | ||
|
|
||
| const alert = await screen.findByRole('alert') | ||
| expect(alert).toHaveTextContent('Invalid project name') | ||
|
|
||
| const cancelButton = screen.getByRole('button', { name: /cancel/i }) | ||
| await user.click(cancelButton) | ||
|
|
||
| expect(alert).not.toBeInTheDocument() | ||
| expect(input.value).toBe('') | ||
| }) | ||
|
|
||
| it('Clears alert and project name on closing dialog', async () => { | ||
| const { user, input, saveButton } = setUp() | ||
|
|
||
| await user.type(input, MOCK_INVALID_PROJECT_NAME) | ||
| await user.click(saveButton) | ||
|
|
||
| const alert = await screen.findByRole('alert') | ||
| expect(alert).toHaveTextContent('Invalid project name') | ||
|
|
||
| const closeButton = screen.getByRole('button', { name: /close/i }) | ||
| await user.click(closeButton) | ||
|
|
||
| expect(alert).not.toBeInTheDocument() | ||
| expect(input.value).toBe('') | ||
| }) |
There was a problem hiding this comment.
Test that clicking "Cancel" button or clicking modal close button result in the same side effects
There was a problem hiding this comment.
Thank you for this thorough testing backfill!
| onConfirm: () => void | ||
| onConfirm: () => unknown | Promise<void> |
There was a problem hiding this comment.
Update type to handle CreateProjectModal passing an async function (onCreateProject) to onConfirm. Using unknown rather than void because Typescript doesn't handle the union type of void and Promise<void>, see here. I wasn't sure if there was a better way to handle this, so happy to adjust as needed!
There was a problem hiding this comment.
Nothing is listening to onConfirm, so it doesn't need to return a promise. You could just do onConfirm: () => void and then in code wrap an asynchronous call in a non-async function.
async function callback(): Promise<void> {
//...
}
//...
return <ConfirmationDialog
onConfirm={() => {
callback()
}}
/>In IntelliJ (not sure about VSCode), this shows a warning that the returned value of the promise is ignored. Apparently, we can use the void operator to return undefined and essentially assert that we don't care about the result. See StackOverflow.
So it seems like the best thing to do would be to use () => void and write the prop as:
<ConfirmationDialog
onConfirm={() => {
void callback()
}}
/>There was a problem hiding this comment.
Got it, thanks for the better solution and explanation! I'll update in the next commit
Before:
After:
