Refactor Modal component

[seancolsen]

Fixes #750

Pavish and I have discussed this PR on a call, so it's best that he review it to pick up where we left off.

Notes

• See the Storybook story Systems/Modal for a place to test out all the different modal types.

Existing Mathesar modals

• This PR only does the minimum amount necessary to make the existing modal instances function correctly.
• Because the Modal component doesn't rely on the central multiplexer to function properly, it actually didn't change much outside the component library.
• The addition of a close button actually makes the existing Mathesar modals look a bit worse in some cases because the title isn't getting set in the right place. I want to fix that in a subsequent PR.

Reflections

After I got the modals working well in Storybook, I set forth trying to clean up the Mathesar modals a bit. To my chagrin however, I realized that some of the code I was hoping to simplify with these modal improvements actually was no better off.

For example in this code

{#if isAddModalOpen}
  <AddEditSchema bind:isOpen={isAddModalOpen}
    isEditMode={activeSchema !== null} schema={activeSchema}/>
{/if}

the {#if} seems unnecessary. But I see now that its purpose is to re-mount the AddEditSchema component.

Also, I was hoping to have Modal contain AddEditSchema instead of the other way around, because that's the pattern I've been used to seeing in the past. I see now that there are some benefits to having AddEditSchema contain Modal.

There are several other small points like this that would be much faster to describe on a call and not worth typing out in here.

Basically, I feel a bit sheepish for coming full circle with this modal design back to a design that's strikingly similar to what we already have. The Modal.svelte file is definitely more robust now, but aside from that, this PR feels a bit underwhelming.

Going forward

I do still want to do some clean up of our existing modals -- and I think I can even address some of the points in the previous section. But I'd like it to be in a subsequent PR. Review/discussion will be more focused. And (more importantly), I'd like to get the toast system merged, and implement #759. With a toast system and a confirmation system, then we can go clean up quite a bit at the same time. For example, some modals have additional state that we can offload to the toast system. And the majority of existing modals can be replaced by the confirmation system. I think it'll be most efficient to do that clean up all in one pass.

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the master branch of the repository
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[seancolsen]

@pavish would you like to take an early look at the direction I'm heading here? I ended up changing course somewhat in the midst of implementing this.

I still have more work to do here within Modal.svelte to support the props and style we discussed, so don't look too closely at that file.

But in ModalMultiplexer.ts and ModalVisibilityStore.ts and Modal.stories.svelte you can see that I'm using a bit of a different pattern to orchestrate the opening and closing of modals. The app would have one ModalMultiplexer instance (either passed through context or imported). Each Modal component instance would use a corresponding ModalVisibilityStore, generated by the ModalMultiplexer and instantiated from within an ancestor of the Modal (or even an imported module). When the modal content and trigger are in separate areas of the component tree, their shared ModalVisibilityStore instance can either be passed through props, context, or imports.

In the ticket, I originally proposed using a string as a way to tie the modal trigger to its content. With that approach, I don't like that it's possible for two separate modals to unintentionally collide by having the same identifying string. With the new direction I'm heading, we avoid that collision risk. We also avoid the requirement to use context for the ModalManager, since the ModalMultiplexer instance can be imported.

It's worth noting that ModalMultiplexer.createVisibilityStore creates a new store for each modal. As we discussed in #776 we'd like to minimize store instances for performance reasons. For example, if we wanted to add the capability to edit a table cell within a modal, then we'd want to find a way to register only one modal on the page instead of hundreds. My intuition is that our previously agreed-upon approach of using unique strings to reference the modal would have similar performance concerns. The plan there was for each Modal instance to register itself with the central ModalManager, winding up with a process similar to that of the store subscription. So in that case too, having hundreds of Modal instances on the page would likely not be acceptable.

[pavish]

@seancolsen and I discussed over a private call and decided on the following:

1. Avoid circular references from the ModalMultiplexer to the ModalVisibilityStore.
   • We can separate out the id generation logic into a util.
   • We will have a higher layer entity that makes use of both the Multiplexer and ModalVisibilityStore. We will only be using this entity outside the internal modal scope.
2. Make the default modal close condition to be only by button, and not by overlay or esc.

[seancolsen]

@pavish this is ready for review now.

[pavish]

@seancolsen This PR looks good to me. As mentioned in description, feel free to take up styling issues in a subsequent PR.

I have a few minor comments here, which I think can also be taken care of in the subsequent PR. I'll leave the call to you.

[pavish]

I think we can allow id to be a string as well

[seancolsen]

Addressed in 1b56cb6

[pavish]

We can add this to eslintrc file, to disable this rule globally.

[seancolsen]

Addressed in 42c4ff4

[pavish]

activeModalStore could be made private.

[seancolsen]

Addressed in 19a40bb

[pavish]

The value returned here should be openModelId instead of undefined.

If modal A is currently open and we do BModalVisibilityStore.update((isOpen) => false); on modal B whose isOpen is already false, it will close the current open modal A.

[seancolsen]

Thanks for catching this! Addressed in 929d300. Though it's worth pointing out that I'm still returning undefined in the case where the modal is currently visible and the updater specifies that the modal should no longer be visible.

[seancolsen]

Thanks for the review @pavish . Even though I added a few more commits after your review, I'm setting this PR to auto-merge because my additional changes are very small and should be in-line with your suggestions.