feat: Add new team editor - button and modal only

[jamdelion]

What is the feature?
Add user in editor: https://trello.com/c/piMcGsJa/2973-add-user-in-editor

What does this PR add?
These changes are UI only!

• adds a button onto the members table that opens a modal.
• The modal does nothing at the moment - both buttons just close the modal.
• The button is hidden behind a feature flag: 'ADD_NEW_EDITOR'

Screenshots:

Button:

Modal:

[github-actions]

Removed vultr server and associated DNS entries

[jessicamcinchak]

Looks good! A couple minor questions, no blockers to merging.

Thanks for clear to-do's and tests, exciting to see this feature taking shape 🙌

[DafyddLlyr]

Fantastic PR - super clear and easy to follow, great level of explanation in the PR description and nice small atomic changes - love it 👍

One very small comment on tests to take a look at please!

[DafyddLlyr]

Wherever possible, we're trying to use the setup() API for consistency. See an example here -

https://github.com/theopensystemslab/planx-new/blob/main/editor.planx.uk/src/@planx/components/AddressInput/Public.test.tsx#L10-L26

Docs:

• https://testing-library.com/docs/user-event/setup/
• https://testing-library.com/docs/user-event/intro/#writing-tests-with-userevent

[jamdelion]

Thanks Daf, I am using the setup API but extracted into a helper for reusability - see helpers/setupTeamMembersScreen.tsx.

Or I have I misunderstood your comment? 🤔

[DafyddLlyr]

Ah sorry - that wasn't a very clear comment in hindsight.

What we would be aiming for is for setupTeamMembersScreen() to return the user generated by setup() so that we can call user.click() as opposed to fireEvent.click() in our tests here 👍

setup() returns Record<"user", UserEvent> & RenderResult so we can access all the values from there, e.g. getByTestId, findByLabelText instead of importing screen.

[jamdelion]

Ah ok, thanks for explaining!

I've updated to be able to get user from setup (and thus got rid of fireEvent but is there a particular reason why we prefer use getBy... functions from here rather than from screen?

I had a go at getting everything from setup but in my opinion it made the tests a lot more unwieldy due to having to initialise the variables globally (rendered) and not being able to destructure them so nicely.

I also think it made it make less sense to have a helper called setupTeamMembersScreen obscuring some of the methods that were exposed. I ended up with this which I don't think is as nice:

describe("when a user with the ADD_NEW_EDITOR feature flag enabled presses 'add a new editor'", () => {
  let rendered: Record<"user", UserEvent> & RenderResult

  beforeEach(async () => {
    rendered = setup(
      <DndProvider backend={HTML5Backend}>
        <TeamMembers teamMembersByRole={exampleTeamMembersData} />
      </DndProvider>
    );
    await rendered.findByText("Team editors"); 
    const teamEditorsTable = rendered.getByTestId("team-editors");
    const addEditorButton = await within(teamEditorsTable).findByText(
      "Add a new editor"
    );
    rendered.user.click(addEditorButton);
  });
  it("opens the modal and displays the input fields", async () => {
    expect(await rendered.findByTestId("modal-create-user-button")).toBeVisible();
    expect(await rendered.findByLabelText("First name")).toBeVisible();
  });
});

In the example you linked, screen is used as well as user so hopefully the way it is left now is acceptable 😅 Super interested in talking through the thought processes behind the current code conventions though so let me know if you'd like a chat!

[jessicamcinchak]

✅ Tests are passing here & I don't want syntax/setup chat to block other dependent PRs, so I've "dismissed the requested changes" from Daf here just so you aren't blocked from merging this one for a couple days until he's back !

Let's pick back up the testing conversation later this week and can always come back for a light refactor for consistency if we decide 👌

Let's revisit test setup syntax when you're back later this week!