Refactor Modal component

[seancolsen]

Problem

Our current Modal component is very simplistic and could use some improvements.

Proposed improvements

• UX:
  • Add a close button in the top right.
  • Close when clicking on the overlay.
• DX:
  • Refactor component design to provide more abstraction in holding the modal's isOpen state, eliminating the need hold that state in local variables within consuming components.
  • Add a title prop.
  • Pass a close function as a slot prop which allows the modal children to close the modal.

Proposed component design

• Within App.svelte, insert:

  setModalManagerInContext();

• Within any consuming component:

  <script lang="ts">
    const modal = getModalManager();
  </script>
  
  <Button on:click={() => { modal.open('addColumn'); }}>Open Modal</Button>
  
  <Modal
    id="addColumn"
    title="Add Column"
    let:close
  >
    <Button on:click={close}>Cancel</Button>
  </Modal>

  The modal trigger and content are coupled only by sharing the same id string, which should be unique across all Modal instances. This decoupling allows the trigger and content to exist far apart from each other in the component tree if needed.

• Within ModalManager.ts:

  /**
   * Each Modal instance registers itself with the ModalManager on mount.
   * The ModalManager is then able to orchestrate the opening/closing of
   * modals globally. We can even add support here for multiple open modals
   * if needed later.
   */
  class ModalManager {
    // TODO
  }
  
  const contextKey = {};
  
  export function setModalManagerInContext(): void {
    const modalManager = new ModalManager();
    setContext(contextKey, modalManager);
  }
  
  export function getModalManager(): ModalManager {
    return getContext(contextKey);
  }

@pavish what do you think of this plan?

[pavish]

@seancolsen The plan look pretty good to me overall. I have a few suggestions:

• Add a close button in the top right.
  Add a title prop.

  Both of these need to be configurable. Since our component is supposed to go in a separate/common package, it needs to support header-less modals. An alternate option would be to support a header slot, and provide a new ModalHeader component.
  In Modal component:

  {#if $$slots.header}
    <div class="header">
      <slot/>
    </div>
  {/if}

  ModalHeader:

  export let title;
  export let allowClose = true;
  ...

• Close when clicking on the overlay.

  I would argue that this is not a good UX for all scenarios. Eg., We may be running a critical operation such as showing the deletion status on the modal, and a mistaken click on the overlay might leave the user confused.

  However, I do think it is good to keep this option configurable. We can have a boolean prop to decide whether or not to close the modal on clicking overlay, where I think it is sensible to keep the default value as false (do not close).

• The ModalManager plan looks great to me. One thing to note:

  • We should assume in the Modal component that the context may not be necessarily set. The component should not have a hard dependency on the manager, it should work even if one is not set.

[seancolsen]

Thanks @pavish

General comments

• I think we should handle the configurability of the title independently from the closing behavior and closing button.

Configuring the close behavior

+1 on all being able to configure the closing behavior. I actually had written some additional specs for that in my original ticket but deleted them so as to keep my suggestion simpler at the start.

What do you think about the following approach?

• Modal has an allowClose prop, true by default. When true, the user can close via button, overlay, or Esc. When false, the modal can only be closed through code.

  • Note: I can't think of a case where we'd want more granular control (e.g. a modal without a close button that can still be closed via Esc), but if you can, then we could use three separate props like allowCloseViaButton etc.

Title

What do you think about the following approach?

• Modal has an optional title prop.
• Modal also has an optional title slot allowing us to put markup in the title if needed (less common).
• If Modal receives the prop and the slot (effectively a developer mistake), it'll render the content from the slot.
• If Modal receives neither the slot or the prop, that's fine too, it'll just render a modal without a title.
  • If in this case the modal also has a close button:
    • This is the trickiest edge case I can identify here.
    • I'd lean towards absolutely position the close button allowing the modal content to begin at the top. It would then be the responsibility of the content to ensure that it doesn't intersect the close button.
    • We could also float the close button.
    • I don't want to overthink it though since I think this will be a pretty rare case.

Why not always us a slot?

• Because a prop is more ergonomic and that's all we need most of the time.

Modal without ModalManager

The component should not have a hard dependency on the manager, it should work even if one is not set.

That's a neat idea. Just to be clear, here's what I'd want to do in that case:

{#if modalIsOpen}
  <Modal on:close={() => { modalIsOpen = false; }}>
    Hi, I'm a modal.
  </Modal>
{/if}

What I mean is:

• The only mechanism we'd have to open and close the modal would be to render or not render the modal from within its parent component.
• To support the workflow of the user closing the modal via button, the modal would emit an event that the parent component would handle.

Events

The above scenario made me realize we probably want on:open and on:close events to always be emitted from the modal. That should be easy to add.

[pavish]

Configuring the close behavior
I can't think of a case where we'd want more granular control (e.g. a modal without a close button that can still be closed via Esc)

• We could use the allowClose flag to determine if the modal can be closed by the user by clicking close button.

  However, I think we need a separate prop for determining if a modal can be closed by clicking overlay. Closing on overlay is generally not preferred for modals where the user is supposed to do configuration related tasks/fill forms etc., even though the modal should have a close button.

  Regarding the esc key, I'm not sure if it's good to tie it up with either of the above.

  A better solution would be to turn this into an array prop instead of a boolean flag. Something like:

  export let closeOn: CloseOn = ['manual', 'esc', 'overlay']
  

  Where the only default option would be manual (which indicates the close button). If the array is empty or if it's null/undefined, it means it can only be closed programmatically.

Title

• This approach sounds good to me. I do have my concerns regarding a header-less modal with a close button, but in such scenarios, we leave it to the developer to style the content accordingly.

Events
we probably want on:open and on:close events

• I believe the existing Modal component already does that.

Modal without ModalManager

• We are probably never going to need to do this, and can rely on the modal manager. But we would require this since our component is supposed to go into a common package for others to use.

  The mentioned approach sounds good, however I do think we would probably need the isOpen prop within the modal component. In your code:

  {#if modalIsOpen}
    <Modal on:close={() => { modalIsOpen = false; }}>
      Hi, I'm a modal.
    </Modal>
  {/if}
  

  The if block should only be here to destroy the modal component completely when it is closed. Even without the if block, the DOM elements should be removed when the user closes the modal, even though that wouldn't destroy the component.

  We cannot rely on the event and then a developer intervention to close the modal (i.e. remove the dom elements). As to when the component needs to be destroyed, is a decision to be taken by the developer for which they could use the events.

  These points are assuming that you're suggesting to remove the isOpen prop completely. It is possible that you have the same thought as me in mind as the points mentioned above. I've just detailed this out so that we're on the same page.

[seancolsen]

@pavish

• I really like your suggesting for making the closeOn prop an array. Let's definitely do that.
• I think the rest of the small details we can work out in the PR phase.

[pavish]

@seancolsen I am removing the technical debt label as this is more of an improvement over existing component. The idea when we build components is that we only build what is immediately required, and then improve upon them as we require more features. Let me know if you have further thoughts on this approach.