Issue 223/delete client modal scott

[xscottxbrownx]

This PR closes #223

This PR:

1. Creates DeleteClientModal component.
2. Renders this component onClick of trash icon in ClientListTableRow.
3. Renames states of AddClientModal component to achieve more clarity.
4. Automatically closes the AddClientModal component once client has been added to the list.

---

Screen.Recording.2023-07-02.at.1.22.35.PM.mov

[andycwilliams]

Approving these changes since my comments are not essential. The code is clean and addresses the related issue.

[andycwilliams]

Minor point, but I notice in the demonstration video that there's a bit of extra spacing on the right side of the modal. That is, "to continue" is wrapped around but the "DELETE CLIENT" button stretches the modal out further to the right. It might be better if the edge of the buttons is made to align with the edge of the rightmost text, wherever that is.

[xscottxbrownx]

Interesting, I didn't notice that previously...

Looks like this modal, the Logout Modal, and the Inactivity Message are all this way. Like the dialog buttons are always over on the right, even if the sx styling is removed. Have to look at theme or how to override for this.

[leekahung]

Oh, one thing I want to touch, do we want to start moving these styled-components over to Material UI now, or have it in a separate PR?

[andycwilliams]

My vote would be to do it now, but am also okay with leaving for another PR.

[xscottxbrownx]

That's Issue #219 assigned to Masen. Pinged them in our Discord DMs.

[leekahung]

My bad, I messed up updating the wrong branch. I've reverted the changes back to your latest commit

[timbot1789]

Could you add a snapshot test for the modal? There are some example snapshots in the tests folder, and I can go over writing them with you if you like.

[xscottxbrownx]

OK,

summary:

1. Renamed states for the AddClientModal to that instead of showModal - due to having another modal in Clients now (DeleteClientModal) just to keep code more clear.
2. Lifted the DeleteClientModal component up to ClientList (now contains the ClientListTable and this DeleteClientModal.)
3. Moved the status notifications to the bottom of the DeleteClientModal instead of the ClientListTable. (This is still planned to be updated to snackbars from MUI in a separate PR/Issue.)
4. Added all the Clients components to its index file.
5. Added test file for this component.

[xscottxbrownx]

Thanks to @timbot1789 for giving a crash course in testing!

I'm still having issues trying to pass my last test that I'd like to implement for this PR. So, I will describe what it is and the approaches I have tried so far - maybe somebody can see the issue, or think of another approach to try.

---

I want to test that the modal does close without taking any action, when the Cancel button is pressed.

---

1) Thought of checking to see if specific elements (that are only in the modal) exist after button click (this one was specifically the delete button. I tried it two different ways:

---

2) Thought of checking to see if the setter function for the state (that controls whether the modal is displayed or not) was fired on button click:

I believe approach 2 to be decent, but it is most likely not written correctly. I don't know how it can correlate my made up closeModal function to the modal actually calling setShowDeleteClientModal state setter function.

---

If anybody sees any issues with the code, has thoughts on a different approach, etc - please ping me with response.

[timbot1789]

@xscottxbrownx Another thing you could do is write a simple wrapper component to launch the modal in, and test that:

import { render, cleanup, waitForElementToBeRemoved } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import React, { useState } from 'react';
import { expect, it, vi, afterEach } from 'vitest';
import { DeleteClientModal } from '../../../src/components/Clients';
import { UserListContext } from '../../../src/contexts';

...


it('closes without an action when cancel button in clicked', async () => {
  const user = userEvent.setup();
  const client = { person: 'tim' }
  const removeUser = vi.fn();
  const ModalWrapper = () => {
    const [showModal, setShowModal] = useState(true)

    return <UserListContext.Provider value={{ removeUser }}>
      <DeleteClientModal
        showDeleteClientModal={showModal}
        setShowDeleteClientModal={setShowModal}
        selectedClientToDelete={client}
      />
    </UserListContext.Provider>
  }

  const { queryByLabelText } = render(<ModalWrapper/>);

  const cancelButton = queryByLabelText("Cancel Delete Client Modal");
  await user.click(cancelButton);
  // wait for the dialog to drop
  await waitForElementToBeRemoved(() => queryByLabelText("Cancel Delete Client Modal"))
  const hiddenCancelButton = queryByLabelText("Cancel Delete Client Modal");
  expect(hiddenCancelButton).toBeNull();
  expect(removeUser).not.toBeCalled();
});

This test seems to work without issue. One thing I had to look up was the waitForElementToBeRemoved function, which does about what it says on the box.

---

However, the React Testing Library docs do include a set of examples, one of which is testing modals:
https://testing-library.com/docs/example-react-modal/

You can see that they do the same test as you wrote. So I'm fine with either approach, but you're right, the test would be more robust if it checks that the modal is actually dropped. Not that it just calls the drop function.

[leekahung]

The changes seems to work fine and the code is running well. I'll approve this.

[andycwilliams]

I know this just closed, but I'm running from the latest Dev branch and am seeing this:

I haven't had time to check what's causing this yet.

[xscottxbrownx]

I know this just closed, but I'm running from the latest Dev branch and am seeing this:

I haven't had time to check what's causing this yet.

Sorry, causing what? I'm not seeing what you are pointing out.

[andycwilliams]

I meant the uncentered buttons. Actually I just realized we have a couple of different philosophies with button placement. On some modals they're centered and on others they're not. Not something we need to address now, if at all.

[timbot1789]

I meant the uncentered buttons. Actually I just realized we have a couple of different philosophies with button placement. On some modals they're centered and on others they're not. Not something we need to address now, if at all.

Sounds like a great first issue for someone new to the project.

[xscottxbrownx]

Yep, see our convo above... it's basically the modals that are just using <Dialog> and <DialogActions> compared to those that have a <form> or <FormSection> inside of them.