Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Modal API #15

Closed
McManning opened this issue Jan 13, 2023 · 2 comments
Closed

RFC: Modal API #15

McManning opened this issue Jan 13, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request v5-alpha

Comments

@McManning
Copy link
Member

UI 5 Modal API

Discussion of a new consistent modal API to cover:

  • a11y wiring
  • level of complexity for inexperienced developers
  • UX restrictions on what a developer can and can't do

Below are several options for APIs.

Modal is controlled by state

Each usage must define a state to link a modal to a trigger.

const [open, setOpen] = useState(false);

(
  // Your trigger
  <Button onPress={() => setOpen(true)}>Open modal</Button>

  // Your modal contents
  <Modal open={open} onClose={() => setOpen(false)}>
    <InformationDialog />
  </Modal>
)

Positives:

Negatives:

  • Developer is expected to be responsible for adding all aria tags and updates. A more realistic and compliant implementation will look like the below example.
    • Increases complexity of code reviews
    • Increases potential for mistakes by inexperienced developers
const [open, setOpen] = useState(false);

(
  // Your trigger
  <Button 
    onPress={() => setOpen(true)}
    aria-expanded={open}
    aria-controls={open ? 'my-unique-id' ? undefined}
  >Open modal</Button>

  // Your modal contents
  <Modal id="my-unique-id" open={open} onClose={() => setOpen(false)}>
    <InformationDialog aria-labeledby="my-unique-id" />
  </Modal>
)

Note: any implementation should still allow experienced developers to break out of common usage and have a controlled modal, as long as the requirements for compliance are very clear.

Modal is controlled by state + required ref

Alternative of the previous example where a ref is required to associate trigger to modal.

const [open, setOpen] = useState(false);
const ref = useRef<Button>(null);

(
  // Your trigger
  <Button 
    ref={ref}
    onPress={() => setOpen(true)}
  >Open modal</Button>

  // Your modal
  <Modal triggerRef={ref} open={open} onClose={() => setOpen(false)}>
    <InformationDialog />
  </Modal>
)

Positives:

  • Can auto-wire aria onto the trigger, developer does not need to know requirements in depth

Negatives:

  • Auto-wiring may work outside of the typical React render flow. I'm not sure if I can consistently ensure the button is still tagged appropriately if it re-renders while the modal is active.
  • Developer is now responsible for the ref and state.

Modal is a button

The modal trigger may only be a button of type ModalTriggerButton and the children of that button is the dialog content.

<ModalTriggerButton label="Open modal">
  {(close) => (
    <InformationDialog onClose={close} />
  )}
</ModalTriggerButton>

Positives:

Negatives:

  • Less flexibility, you can only use buttons
    • but that is a more consistent UX

Modal is a slot host

The modal is a host component for content slots.

<ModalHost>
  <Slot name="trigger">
    <Button>Open modal</Button>
  </Slot>
  <Slot name="dialog">
    <InformationDialog />
  </Slot>
</ModalHost>

// OR - the subcomponents-style API.
// Architecturally - they're the same:

<Modal>
  <Modal.Trigger>
    <Button>Open modal</Button>
  </Modal.Trigger>
  <Modal.Dialog name="dialog">
    <InformationDialog />
  </Modal.Dialog>
</Modal>

Positives:

Negatives:

  • Verbosity
  • Additional work on the host component to ensure colocation of children.
    • For example, if not carefully implemented the host trigger may end up several components deep within a host, or several sibling components could be between trigger and dialog.
    • Not a huge problem, we establish this as a design pattern already for components that only support with collection <Item> children, e.g. lists and checkbox sets.

Modal trigger is a render prop

Modal host acts as a functional linkage and the element to trigger the modal (button or whatever) is a render prop.

<ModalHost trigger={<Button>Open modal</Button>}>
  <InformationDialog />
</ModalHost>

Positives:

  • Flexible trigger element
  • Can automatically wire up required aria to the trigger element

Negatives:

  • ??

Modal is a render prop

Inverse of the prior example, the host wraps the trigger and the modal acts as the render prop:

<ModalHost modal={<InformationDialog />}>
  <Button>Open modal</Button>
</ModalHost>

Positives:

  • ??

Negatives:

  • Most content work will be done within the modal and not the trigger, so you'd either have an awkward inline component or you have to make a separate dialog component to write content in.
<ModalHost modal={
  <InformationDialog title="Terms of service">
    Long content would go here
  </InformationDialog>
}>
  <Button>Open modal</Button>
</ModalHost>

// vs

function TermsOfServiceDialog() {
  return (
    <InformationDialog title="Terms of service">
      Long content would go here
    </InformationDialog>
  )
}

<ModalHost modal={<TermsOfServiceDialog />}>
  <Button>Open modal</Button>
</ModalHost>

// vs the inverse example where trigger is the render prop:

<ModalHost trigger={<Button>Open modal</Button>}>
  <InformationDialog title="Terms of service">
    Long content would go here
  </InformationDialog>
</ModalHost>
@McManning McManning added the question Further information is requested label Jan 13, 2023
@McManning McManning self-assigned this Jan 13, 2023
@McManning
Copy link
Member Author

Some feedback from the accessibility folks:

  • Don't use Adobe's aria tags on the trigger button, it's confusing and not really standard practice. Our dialogs/modals will always require attention/interaction anyway, since they are just for confirmations, forms, and other information for intentional user review. We very intentionally don't support popup spam.
  • Don't support esc on form modals - can be accidentally pressed too easily and end up resetting large forms
  • Do return focus to the trigger once the modal is gone, which does eliminate option 1 without the ref as a candidate.

@McManning McManning added enhancement New feature or request v5-alpha and removed question Further information is requested labels Jan 13, 2023
@McManning
Copy link
Member Author

Obsolete in beta.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v5-alpha
Projects
None yet
Development

No branches or pull requests

1 participant