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 poc #1464

Closed
wants to merge 3 commits into from
Closed

New dialog poc #1464

wants to merge 3 commits into from

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Feb 16, 2024

Part of #1461

This PR is a proof of concept of what could be a new component to render native dialog elements.

It covers the basic functionality, with an open prop that triggers the dialog.show() side effect when true, or dialog.close() when false.

It also takes into consideration how regular dialogs and modal dialogs should be displayed.

Finally, it does not make assumptions about how children should be rendered, rendering what's provided verbatim.

For cases where we want to render the dialog as a panel/card, we can consider adding a new component that wraps children in a panel and then nests that into the new dialog.

Open questions

  • I reused the modal prop for modal sizes, because it was convenient, and the resulting API seems intuitive: modal for an auto-size modal dialog, and modal="lg" for a large-size modal dialog.
    Should we have a size prop that can be used with non-modal dialogs as well? Should we have a separate ModalDialog that handles sizes?
  • Should we have a fallback for browsers not supporting dialog elements? Perhaps we could define a component wrapping the new dialog and the old one and orchestrating which one to render based on browser support.
  • Should we drop support for the closeOnFocusAway prop? It is false by default and is not being redefined in any downstream project.
    Also, you cannot focus away when opening the native dialog as modal, and when it is not a modal you will likely want to keep it open.
  • Should we drop support for closeOnEscape? It is true by default for modal dialogs and false for regular dialogs.
    Native dialogs implement this natively when opened as modal, which matches that behavior.
    There's only one use case in which we are currently using a non-modal dialog which can be closed with Esc, which is the grade comment popover. In this case we are using the dialog because it's convenient, but it is actually implementing the popover pattern, which is different.
  • Should we drop support for closeOnClickAway? It's false by default and like in previous point, we are redefining this behavior only for the grade comment popover.

TODO

  • Decide if we want to support closeOnClickAway, and implement it if so
  • Decide if we want to support closeOnFocusAway, and implement it if so
  • Decide if we want to support closeOnEscape, and implement it if so
  • Implement initialFocus
  • Implement restoreFocus
  • Implement transitionComponent
  • Test the component with real use cases in downstream projects

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (06ffc26) to head (314f85f).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1464   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           60        60           
  Lines          933       933           
  Branches       351       351           
=========================================
  Hits           933       933           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@acelaya
Copy link
Contributor Author

acelaya commented Feb 29, 2024

I have been trying to adopt the TransitionComponent API as we did with previous dialog components, and there are two considerations:

  1. TransitionComponents were build in a generic way, which is very flexible, but adopting them dynamically adds a lot of complexity.
  2. The differences between how this new dialog works internally and the previous ones mean we have to spend time solving the same problem again. It was not easy then, and I haven't nailed it yet. In fact, this one presents new problems that might be trickier to address.
  3. It still has the same problems we faced with transition components in previous dialogs.

Because of this, I'm thinking on a less flexible but simpler solution, which implies adding CSS transitions directly to the dialog, as explained in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

This would add less flexibility, as every type of transition would need to be implemented via tailwind classes and config.

However, since TransitionComponents were created we have used only one implementation, the Slider, so we could start with that.

@acelaya acelaya closed this May 9, 2024
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.

1 participant