Make Dialog API more strict depending on variation

[acelaya]

This PR has been motivated by current requirement to pass some title (like title="") when using variant="custom" in Dialog, even though that title is then ignored.

Context

When the Dialog component was created, it was designed to wrap a Panel, and therefore, it extended some props from it that were later forwarded to the wrapped Panel.

Later, there was a use case to render a simpler Dialog where having to deal with a panel was inconvenient, and we introduced the variant prop, which could have values 'panel' | 'custom', being panel its default, and making provided children be rendered verbatim (not wrapped in a Panel) when value custom was provided.

However, the rest of the props API was the same, making some of the required Panel props still be required for Dialog, even when there's no Panel involved if variant="custom".

Actions taken

This PR updates the Dialog props so that its defined as a union type, where the variant prop is used to determine if the Panel props are required or not.

This way, we can continue defining panel dialogs, if no variant is provided, or it is provided with value panel:

<Dialog title="Some dialog" buttons={...} icon={...} />

<Dialog variant="panel" title="Some dialog" buttons={...} icon={...} />

On the other hand, we can define custom dialogs where the panel props won't be required (in fact, they won't be allowed)

<Dialog variant="custom" closeOnEscape restoreFocus />

Considerations

The solution proposed here is the more resilient for consumers, as you will always be forced to provide a title as long as variation is panel, while you will be reminded no title can be used for custom dialogs.

However, it makes the Dialog implementation a bit complex, due to the mix of Panel props, Dialog props and HTML attributes, which requires a couple of type guard checks and multiple prop unpacking.

A more pragmatic solution would be to just define title as optional, and set a default fallback value when passing it down to the Panel.

However, this solution is more prone to mistakes on consumers.

[codecov]

Codecov Report

Merging #1256 (47c2d14) into main (be104fc) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1256   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           61        61           
  Lines          837       842    +5     
  Branches       330       331    +1     
=========================================
+ Hits           837       842    +5     

Files	Coverage Δ
src/components/feedback/Dialog.tsx	100.00% <100.00%> (ø)
src/components/feedback/ModalDialog.tsx	100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[robertknight]

These changes effectively document the actual the current Dialog API better, so that's a plus.

Some alternative approaches that we could use to simplify the Dialog API in future might be:

1. Make the consumer responsible for composing Dialog and Panel. Dialog does pass some defaults along to the Panel, that could be done internally by using cloneElement though.
2. Since the dialog-with-panel is an extension of the custom dialog, you could create a separate component for it (<PanelDialog ...> instead of <Dialog variant="custom">)

These are all more disruptive changes to downstream consumers though, so this PR is a useful intermediate step.

[robertknight]

An alternative approach to forwarding all the props would be to put all the panel props under a specific key. This would be a little less ergonomic to use in the common case that the dialog is a panel though.

[robertknight]

An alternative approach would be to make consumers responsible for composing the dialog and panel. It looks to me like Dialog is mostly just forwarding props here, plus setting a few additions and defaults (fullWidthHeader). This would be a more disruptive change to downstream consumers though.

[acelaya]

2. Since the dialog-with-panel is an extension of the custom dialog, you could create a separate component for it (<PanelDialog ...> instead of <Dialog variant="custom">)

Yep, this is probably how the Dialog should have been designed in the first place, but it was not easy to tell at first. It has become more obvious now that we have something in front of us.

I even remember discussing this with Lyza.