-
Notifications
You must be signed in to change notification settings - Fork 255
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): fix menuItems cloning #1457
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's much better. The only thing that will not work will be if the developer compares the event target of the menu item click event with references of the original items. I don't know if this is a realistic use case, but we can always do this later. This will not invalidate any of the code, proposed with this change, as we always need the ovserver. Good job!
packages/fiori/src/ShellBar.js
Outdated
} | ||
this._updateClonedMenuItems(); | ||
this.updateStaticAreaItemContentDensity(); | ||
this.menuPopover.openBy(this.shadowRoot.querySelector(".ui5-shellbar-menu-button")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended that this line is outside of the if
? So we always open the popup even though it's empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, this is by mistake when I extracted the code to _updateClonedMenuItems
@@ -641,6 +642,7 @@ class ShellBar extends UI5Element { | |||
} | |||
|
|||
onExitDOM() { | |||
this.menuItemsObserver.disconnect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you disconnect it here (f.e. the dev called removeChild(shellbar), are we sure _observeMenuItems() will be called again again upon re-inserting into the DOM? I would guess so, but just to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should work, I read this in the DOCS:
disconnect() stops the MutationObserver instance from receiving further notifications until and unless observe() is called again
packages/fiori/src/ShellBar.js
Outdated
@@ -819,6 +821,34 @@ class ShellBar extends UI5Element { | |||
} | |||
} | |||
|
|||
_updateClonedMenuItems() { | |||
if (!this.menuItems.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no more items, shouldn't we let it delete all clones too (so the next line should always run)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I should just remove the return I think, then the array with clones will be empty and the forEach just wont run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected.
We used to just copy the text of the menuItems within the Light DOM of the ui5-shellbar, and not only the attributes of those items are missing, but also we don't update the internal items whenever the original ones change. For example: if the textContent change - it is not applied to the internal one. Adding a mutation observer keeps everything in sync. Currently we can not just forward the outside slot to the List, that is inside a Popover, that is in the static area.
Fixes: #1442