fix(aria/combobox): dialog popup support #32279
Conversation
wagnermaciel
added
target: rc
|
Deployed dev-app for 0d57b47 to: https://ng-dev-previews-comp--pr-angular-components-32279-dev-48uujtwx.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
| } | ||
|
|
||
| @Directive({ | ||
| selector: 'dialog[ngComboboxDialog]', |
There was a problem hiding this comment.
Is the dialog meant to be positioned relative to a trigger element or will it always be centered on the page?
There was a problem hiding this comment.
The dialog can either be centered or have special positioning depending on the use case. The positioning of the dialog is left up to developers
|
|
||
| afterRenderEffect(() => { | ||
| if (this.element) { | ||
| this.combobox._pattern.expanded() |
There was a problem hiding this comment.
I think that by default native dialogs can be closed by clicking outside. I think we might end up in a situation where the expanded state of the pattern isn't in sync with the dialog's expanded state.
There was a problem hiding this comment.
This is handled by the ComboboxDialogPattern, but in an unintuitive way. I went back and added a comment to clarify. The ComboboxDialogPattern has an onClick listener that handles this case. If the user clicks outside of the dialog, the click event fires on the dialog element.
|
|
||
| const popupControls = this.inputs.popupControls(); | ||
|
|
||
| if (popupControls instanceof ComboboxDialogPattern) { |
There was a problem hiding this comment.
It's worth noting that cases like this will prevent the dialog pattern from being tree shaken.
| return; | ||
| } | ||
|
|
||
| if (popupControls instanceof ComboboxDialogPattern) { |
There was a problem hiding this comment.
It seems like we need special casing like this in several places. Would it make sense to just have a different combobox component?
There was a problem hiding this comment.
I agree. The more popup cases that get handled the more complex the internals of the combobox are getting because of all the special casing.
I am considering an alternative approach which would simplify the implementation. The tldr is there should be multiple ComboboxPattern definitions and the wrapper Combobox directive should instantiate the right one depending on what popup it has.
I believe this change can happen in-place without changes to the public api, which is why I still sent out this PR
src/components-examples/aria/combobox/combobox-dialog/combobox-dialog-example.ts
Show resolved
Hide resolved
src/components-examples/aria/combobox/combobox-dialog/combobox-dialog-example.css
Show resolved
Hide resolved
* fix(aria/combobox): dialog popup support * fixup! fix(aria/combobox): dialog popup support * fixup! fix(aria/combobox): dialog popup support * fixup! fix(aria/combobox): dialog popup support * fixup! fix(aria/combobox): dialog popup support * fixup! fix(aria/combobox): dialog popup support * fixup! fix(aria/combobox): dialog popup support (cherry picked from commit f287cd6)
2 participants
No description provided.