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-shellbar): do not duplicate popover menu items #1456

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

vladitasev
Copy link
Contributor

The _menuPopoverItems metadata property had a wrong type and a wrong default value. When the default value is an object, in this case [], whenever there is a type mismatch, the default value is returned: return propDefaultValue; in UI5Element.js. However, if the default value is an object, it's passed by reference and when the components work with it (f.e. push items as is here), the defaultValue is updated every time. Thus items are duplicated again and again.

In addition, to prevent this from happening in the future, some more metadata checks are added:

  • type can no longer be Array
  • properties of type equal to Object, or with multiple: true, cannot have a default value at all. The default value can only be {} or [] respectively, and provided by the framework.

In addition, the check for Boolean properties was wrong, a leftover from the time when Boolean was denoted by a string "boolean".

Copy link
Collaborator

@MarcusNotheis MarcusNotheis left a comment

Choose a reason for hiding this comment

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

Looks good to me, I was not able to reproduce the issue.

@MarcusNotheis
Copy link
Collaborator

While looking on the code again I would have another question on the following lines from the ShellBar constructor: Is there any specific reason why you copy the text content fo the menuItems into the private aggregation? Wouldn't it be easier to just use menuItems slot as popover content?

this._header = {
  press: (event) => {
    if (this.menuItems.length) {
      this._menuPopoverItems = [];
      this.menuItems.forEach((item) => {
        this._menuPopoverItems.push(item.textContent);
      });
      this.updateStaticAreaItemContentDensity();
      this.menuPopover.openBy(this.shadowRoot.querySelector('.ui5-shellbar-menu-button'));
    }
  },
};

@vladitasev vladitasev merged commit ae20272 into master Apr 9, 2020
@vladitasev vladitasev deleted the fix-shellbar-item-duplication branch April 9, 2020 13:32
@vladitasev
Copy link
Contributor Author

@MarcusNotheis The reason is that since RC.6 all popups, opened by other components, are in a ui5-static-area-item component on top body level. Before that, they were parts of the higher-order components, hence it was possible to transitively slot children to them: shellbar slots menu items to the popover. However, ever since the popovers were detached from components and moved to the static area, naturally children, slotted to say shellbar, cannot be transitively slotted to something outside of its DOM. Therefore the only option is to go for workarounds. We are currently improving the "cloning" logic so that it's completely transparent for applications.

The reason to move all popups to a static area is a specific HTML/CSS behavior that many developers find unintiutive, but nonetheless is by the specifications and this is how all browsers currently behave. It is that elements, positioned with position: fixed are shifted if some HTML element in DOM tree above them, has transform. Therefore the only reliable way in HTML to position an element to an arbitrary element, without risks of overflow: hidden cuts or shifting due to transform somewhere in the DOM tree above, remains to have a top-level (body) static area and render popups there.

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.

4 participants