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

fix: make static are items recognizable to openui5 dialogs #5888

Merged
merged 4 commits into from
Oct 7, 2022

Conversation

ilhan007
Copy link
Member

@ilhan007 ilhan007 commented Oct 5, 2022

Issue
When UI5WC ui5-date-picker is opened inside UI5 dialog, click "ESC" wrongly close whole dialog.

Root cause
It's an integration issue between UI5WC Popups and OpenUI5 Popups - the focus never gets to the DatePicker's popover as it's not recognized as an external content by the sap.m.Dialog.

Solution
While we are working on more general, out of the box solution by integrating UI5WC Popups and OpenUI5 Popups, we would like to provide the following solution for the time being:

Forwarding the "data-sap-ui-integration-popup-content" attribute from the component that opens popup to its the static are item, which makes the static are item an external content for the sap.m.Dialog (as the aforementioned attribute is already part of the OpenUI5 logic). When the attribute is present, the focus goes inside the DatePicker's popover and the ESC behaviour works as expected.

Related to: #5634

@ilhan007 ilhan007 requested review from pskelin and vladitasev October 5, 2022 08:38
@@ -17,6 +17,7 @@ class StaticAreaItem extends HTMLElement {
this._rendered = false;
this.attachShadow({ mode: "open" });
this.pureTagName = "ui5-static-area-item";
this.popupIntegrationAttr = "data-sap-ui-integration-popup-content";
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to have this as a constant in the module directly

this._updateContentDensity();
this._updateDirection();
this._updateAdditionalAttrs();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is not a problem at all, but it would be safest IMO to move this method call to where the old this.setAttribute was (before updateContentDensity and updateDirection) so that we don't break any functionality in these methods that relies on this.pureTagName, if such exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree 👍

@ilhan007 ilhan007 requested a review from vladitasev October 6, 2022 11:26
@ilhan007 ilhan007 merged commit efaa1d6 into main Oct 7, 2022
@ilhan007 ilhan007 deleted the popups-openui5-wc-integration branch October 7, 2022 07:12
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.

2 participants