✨ Add new hook for modal state

[ibolton336]

• Adds a new hook for handling modal state

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (5e8f6c7) 48.10% compared to head (29085ac) 48.10%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #931   +/-   ##
=======================================
  Coverage   48.10%   48.10%           
=======================================
  Files         177      177           
  Lines        4232     4232           
  Branches      900      900           
=======================================
  Hits         2036     2036           
  Misses       2183     2183           
  Partials       13       13           

Flag	Coverage Δ
unitests	48.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gildub]

Could we go further to provide a hook that can be used for any modal, not only create/edit but also delete, bulk, or some other action ?

export function useModalState<T, K = "">(action?: string) {
  const [modalState, setModalState] = React.useState<K | T | null>(null);
  const isModalOpen = modalState !== null;
  const resourceAction = modalState !== action ? modalState : null;

  return {
    modalState,
    setModalState,
    isModalOpen,
    resourceAction
  };
}

which would be consumed as:

  const {
    modalState: creditEditModalState,
    setModalState: setCreditEditModalState,
    resourceAction: applicationToUpdate
  } = useModalState<Application, "create">("create");

or

  const {
    modalState: deleteModalState,
    setModalState: setDeleteModalState,
  } = useModalState<Application>();

[gildub]

The delete state doesn't belong here.

[mturley]

I think I agree that the delete state should be its own thing and used to determine whether to show a confirm delete modal (separate from the create/edit modal we control here).

[mturley]

@gildub I see where you're going with that further abstraction, but I think it might be a little too abstract. The hook is useful because the create/edit modal in particular is weird (we have one modal/form for two actions), and I don't think the delete modal requires that complexity (you can just have const [resourceToDelete, setResourceToDelete] = React.useState<T | null>() on its own in the page and use !!resourceToDelete as your modal's isOpen). I think if Ian is going to use this discriminated union type we discussed on Slack:

type ModalState<T> =
    | { mode: "create"; resource: null }
    | { mode: "create"; resource: T }
    | { mode: "edit"; resource: T }
    | null

(which @ibolton336 we might want to rename to CreateEditModalState<T>)

then you have more information that is specific to creating and editing: you can be in the initial creation step with no resource, or you can be in the creation step but you have a "draft" resource that already exists (we created something but we haven't submitted the modal yet), or you can be in edit mode, or the modal is closed. With this type, if you try to have { mode: "edit", resource: null } that's a type error.

I think it's difficult to preserve that strictness and also abstract this into something that is useful beyond the create-and-edit use case. If we find another use case that makes the idea of factoring something out here more compelling we can always do that in the future.

[gildub]

We actually have other cases with actions like bulk delete or bulk export.
I found those in the migration waves, so we want to pass a list (migrationwaves[]) and we have the delete action.

[mturley]

Isn't that the same case as single delete, but with an array of resources instead of a single resource?

const [migrationWavesToDelete, setMigrationWavesToDelete] = React.useState<MigrationWave[] | null>(null);
const isBulkDeleteModalOpen = migrationWavesToDelete?.length > 0;

I don't think we need a hook abstraction for that and if we do I don't think it matches this one. The specific strange thing we're abstracting away is that we have state which could be a special action ("create", which has no resource associated), or a resource, or null. This is just resource or null.

[mturley]

LGTM! Nice