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

New Dialog component wrapping a native dialog #1461

Open
acelaya opened this issue Feb 13, 2024 · 0 comments
Open

New Dialog component wrapping a native dialog #1461

acelaya opened this issue Feb 13, 2024 · 0 comments
Labels
component concerning a UI component that is part of the package API

Comments

@acelaya
Copy link
Contributor

acelaya commented Feb 13, 2024

The support for the native dialog is reaching all the browsers we support. This is a good opportunity to build a new Dialog component that makes use of it, and allows us to drop some of the custom logic.

We could just refactor the existing Dialog and ModalDialog components, but creating a new one instead, would allow us to apply some architectural changes and not having to keep a 100% compatible API, while we slowly adopt it in downstream projects.

Some of the changes I think this new component should introduce:

Handle its open/closed state via an open prop, instead of via mounting/unmounting.

This will better fit the side effects we need to trigger in order to show/hide the native dialog.

It will also help solving an existing problem with current components when transition components are passed. We tried to solve that problem this way some time ago, and it proved challenging, but with a simplified component it will probably be easier to handle.

Ultimately, most of the component libraries we usually get inspiration from, use some kind of prop handled by consumer code to determine if the component should show or not.

Do not implicitly wrap a Panel.

Existing Dialog started as a wrapper around Panel, which became a problem later when we tried to use it for other components providing its own "layout".

Instead, we should have a simpler Dialog, and if needed, a higher level PanelDialog wrapping the new Dialog and implicitly rendering a Panel or Card.

Potential API changes that we might want to introduce, but need some testing and experimentation first:

Single Dialog component for modal and non-modal contexts.

Due to the amount of custom logic in existing implementations, we have two separate components, Dialog and ModalDialog.

Thanks to the amount of logic we will be able to get rid of when using the native dialog, maybe we can have a single component with a modal={true|false} prop.

Use cases

An inspiration for this component can be found in the client, where we recently used the native dialog with a very small fraction of the API we might want to support, to render the notebook and the user profile: hypothesis/client#6207

@acelaya acelaya added the component concerning a UI component that is part of the package API label Feb 13, 2024
@acelaya acelaya mentioned this issue Feb 16, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component concerning a UI component that is part of the package API
Projects
None yet
Development

No branches or pull requests

1 participant