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

expose show()/close() methods via Dialog.useInstance() #1983

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Apr 4, 2024

Changes

This adds two new API surfaces to Dialog:

  1. A useInstance hook containing show()/close() methods for imperative manipulation of dialog.
  2. An instance prop which accepts an instance returned from the above hook.

Together, this makes it easier to control the dialog without explicitly maintaining its state.

Supersedes #1976

Usage:

const dialog = Dialog.useInstance();

<Button onClick={dialog.show}>Open dialog</Button>
<Dialog instance={dialog}>
  <Button onClick={dialog.close}>Close dialog</Button>
</Dialog>

The implementation is quite simple: useInstance will create an instance of Instance, and useSynchronizeInstance will simply add methods to it. The instance stays referentially stable throughout its lifetime.

Testing

Added unit test and e2e test, updated stories, updated website examples.

Docs

Added docs + jsdocs + changeset.

@mayank99 mayank99 self-assigned this Apr 4, 2024
@mayank99 mayank99 force-pushed the mayank/dialog-instance branch from 022f304 to 5021dba Compare April 4, 2024 23:18
@mayank99 mayank99 force-pushed the mayank/dialog-instance branch from 5021dba to d8a858c Compare April 4, 2024 23:21
@mayank99 mayank99 changed the title add instance.show() and instance.close() methods to Dialog expose show()/close() methods via Dialog.useInstance() Apr 5, 2024
@mayank99 mayank99 marked this pull request as ready for review April 5, 2024 14:29
@mayank99 mayank99 requested review from a team as code owners April 5, 2024 14:29
@mayank99 mayank99 requested review from r100-stack and Ben-Pusey-Bentley and removed request for a team April 5, 2024 14:29
apps/website/src/content/docs/dialog.mdx Outdated Show resolved Hide resolved
examples/Dialog.draggable.jsx Outdated Show resolved Hide resolved
Copy link
Contributor Author

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to hold off on merging this one for now.

I'm working on adding similar apis to Modal, Popover, Tooltip, DropdownMenu and InformationPanel. All of them should probably release together.

I'm also experimenting with an invoker-like declarative API for creating tight associations between dialogs/popovers and their triggers. For example:

const dialog = Dialog.useInstance();

<Button invoketarget={dialog}>Open</Button>
<Dialog instance={dialog}>
   <Button invoketarget={dialog}>Close</Button>
</Dialog>

If we want to release things faster without leaving PRs open for long times, maybe we should mark these new APIs as "alpha" or "unstable".

@r100-stack r100-stack mentioned this pull request Aug 6, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants