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

Controlled vs uncontrolled lightbox #43

Open
dvoytenko opened this issue Nov 1, 2019 · 0 comments
Open

Controlled vs uncontrolled lightbox #43

dvoytenko opened this issue Nov 1, 2019 · 0 comments
Labels
TBD To be discussed/design decisions

Comments

@dvoytenko
Copy link
Contributor

AMP's lightboxes are components with completely separate "roots" and lifecycle. They only supply imperative APIs such as:

lightbox.open().then(result => {
  // Lightbox has closed with a possible return value.
});

A classical approach in React would use a controlled style:

export function Demo() {
  const [open, setOpen] = React.useState(false);
  return (
    <div>
      <Button onClick={() => setOpen(true)}>Open</Button>
      <XDialog open={open} onClose={_ => setOpen(false)}>...</XDialog>
    </div>
  );
}

There are some benefits when using a controlled style. But there are also some nuances to consider:

  1. Open state has to be controlled. This expands the required API surface. I.e. to use the XDialog, one must manage open state, supplied as a property, and handle onClose.
  2. Out-of-lightbox closing has to be non-vetoable. This can probably be remedied by an additional onCloseRequest event callback.
  3. AMP element itself has to be uncontrolled since DOM elements do not have controlled/uncontrolled concept. Thus we'd have to remap the controlled state to imperative DOM APIs.
@dvoytenko dvoytenko added the TBD To be discussed/design decisions label Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TBD To be discussed/design decisions
Projects
None yet
Development

No branches or pull requests

1 participant