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(ui5-popover): fix closing order of popovers #1676

Merged
merged 7 commits into from
Jun 10, 2020
Merged

Conversation

fifoosid
Copy link
Contributor

@fifoosid fifoosid commented May 22, 2020

Fixes #1628

The problem was that there was no knowledge whether a popover is opened inside another popover and every time a popover was closed, all popovers on the whole page were closed. Now if a popover is closed only the popovers open within it are closed

@fifoosid fifoosid changed the title [WIP]fix(ui5-popover): fix closing order of popovers fix(ui5-popover): fix closing order of popovers May 22, 2020
let currentElement = instance.parentNode;
const parentPopovers = [];

while (currentElement.parentNode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should do this. Traversing the DOM upwards and checking for a tag name is a prerequisite for many problems down the road. People can have their own popover-like components with different tag names, some might even not inherit from popover directly.

The point of the popup registry is to keep track of exactly this kind of information: whenever a new one opens, it registers itself, so we know which ones are below and which ones above. So to me the solution here is to completely use the registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the fact that traversing the DOM is not a good thing.

In the case when someone makes popover-like component, with different tag and/or not extending ui5-popover the current registry is NOT going to care of it as well, so from this POV we are not going to brake anything we have supported.

The current goal is to make each ui5-popover aware whether it is opened inside of another one or not.

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

Now Esc no longer closes subsequent popovers after you press it once. For example, in the case when you have 3 opening from one another, after you close the first one, nothing happens after you hit Esc for the second, or third time

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

I can approve, just fix the build.
Ohterwise, my comment is not so important, we can fix this later with the other change too.

packages/main/src/popup-utils/PopoverRegistry.js Outdated Show resolved Hide resolved
@fifoosid fifoosid merged commit 14add07 into master Jun 10, 2020
@fifoosid fifoosid deleted the close-popover branch June 10, 2020 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui5-popover: Inconsistent closing behaviour with multiple instances
2 participants