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

[Menu & MenuItem | 1.14.0]: The Dynamic Menu is not working on the second sub-level #7391

Closed
1 task done
rv97 opened this issue Jul 31, 2023 · 22 comments
Closed
1 task done
Assignees
Labels
bug This issue is a bug in the code High Prio TOPIC B

Comments

@rv97
Copy link

rv97 commented Jul 31, 2023

Bug Description

When the main menu has two levels of sub-menu where the second level sub-menu should be rendered dynamically, then the loading indicator is stuck and does not render the content automatically. We need to go one level back and then open the sub-menu to show the rendered content.
The same works in the case of the first-level sub-menu.

Affected Component

Menu, MenuItem

Expected Behaviour

How many ever sub-levels are there, the busy indicator should be gone automatically and render the content.

Isolated Example

https://codesandbox.io/s/prod-bird-mjtv35?file=/src/App.js

Steps to Reproduce

  1. Open the sandbox link shared.
  2. Click on "Show Menu"
  3. Open the "New File" sub-menu.
  4. Again open the "Open" sub-menu
    ...

You can observe that the sub-menu inside a sub-menu does not render the content automatically.

Log Output, Stack Trace or Screenshots

No response

Priority

High

UI5 Web Components Version

1.14.0

Browser

Chrome

Operating System

MacOS

Additional Context

No response

Organization

SAP Labs

Declaration

  • I’m not disclosing any internal or sensitive information.
@rv97 rv97 added the bug This issue is a bug in the code label Jul 31, 2023
@unazko unazko self-assigned this Jul 31, 2023
@unazko
Copy link
Contributor

unazko commented Jul 31, 2023

Hello @SAP/ui5-webcomponents-topic-b,

This issue is for us.

Best regards,
Boyan

unazko added a commit to unazko/ui5-webcomponents that referenced this issue Aug 7, 2023
Issue:
- The sub-menu busy state and dynamically loaded
items weren't proeprly updated.

Fixes: SAP#7391
@rv97
Copy link
Author

rv97 commented Nov 27, 2023

Hi @unazko ,

Is there any update on this?

@unazko
Copy link
Contributor

unazko commented Feb 12, 2024

Hi @rv97,

Apologies for the late reply. We've got stuck with merging the previously related PR.
Currently there is a new related PR, which will most probably fix this issue.
We're currently testing if the issue will be resolved with the PR.

Best regards,
Boyan

unazko added a commit to unazko/ui5-webcomponents that referenced this issue Feb 28, 2024
- The ui5-menu elements used for sub-menus are created only once
and are being reused afterwards. They are no longer destroyed on close.
This contributes to lowering the count of the slow DOM manipulation operations.

- There is now no differentiation between mobile and desktop
device in regards to the display mechanism. In both cases
we rely on the template to do the job as the components used
for composition like ui5-list and ui5-responsive-popover do
comply with the device.

Fixes: SAP#7767
Fixes: SAP#7423
Fixes: SAP#6761
Related to: SAP#7391
@unazko
Copy link
Contributor

unazko commented Mar 5, 2024

Hello @rv97,

I've managed to fully test the provided sample and unfortunately the previously linked PR's aren't going to fix this issue:
I suspect you're already using the following workaround:
https://codesandbox.io/p/sandbox/boring-solomon-92qknd?file=%2Fsrc%2FApp.js

There is a workaround at least. We're going to continue debugging this behavior.

Best regards,
Boyan

@shubhamnazare
Copy link

Hi @unazko,

I'm from @rv97' team working on this issue. The workaround you proposed is not working in our case. We have a complex reusable component which is used to render all the menus across the entire application. Introducing the workaround in our application brings in a lot of code refactoring and unnecessary errors. Hence, we request the UI5 Web Components Team to check if it is possible to fix this issue in the Menu component itself.

Thanks & Regards,
Shubham.

@DMihaylova
Copy link

Hi @rv97 & @shubhamnazare,

We're actively addressing your concern and will update you shortly.
Thank you for your patience!

Regards,
Diana

@rv97
Copy link
Author

rv97 commented May 9, 2024

Hi @DMihaylova ,

Thanks for the update.

Regards,
Vignesh R

unazko added a commit that referenced this issue May 21, 2024
The `ui5-menu-item` now extends `ListItem` abstract class and it
will be represented directly as a list item in the DOM.
The application developers could now add custom styles and attach native event handlers
to the `ui5-menu-item` as it is no longer an abstract class, but a physical component:

```html
<ui5-menu-item id="exitItem" text="Exit" style="border: 2px solid teal" icon="journey-arrive"></ui5-menu-item>
```

```ts
document.getElementById("exitItem").addEventListener("focusin", () => {
    ...
})
```

Related to: #8461
Related to: #7391
@unazko
Copy link
Contributor

unazko commented May 22, 2024

Hello @rv97,

The issue is fixed with the following PR (#8722) for the latest ui5-webcomponents release.

We'll have to develop separate fix for 1.24 release though as the upper change is a major refactoring of the ui5-menu.

Best regards,
Boyan

@rv97
Copy link
Author

rv97 commented May 24, 2024

Thanks @unazko. Please let us know once you have finalized the version this fix would be available from.

Thanks,
Vignesh R

@unazko
Copy link
Contributor

unazko commented Jun 4, 2024

Hi Vignesh R,

This behavior will be fixed with webc v2 currently. We'll be working on a transport as a separate fix for 1.24.x releases.
From the following URL you could see the release candidate for v2 marked with the @next tag:
https://www.npmjs.com/package/@ui5/webcomponents?activeTab=versions

You could perform internal testing with the release candidate (2.0.0-rc.4).

There is also information about the incoming releases down on the following page: https://github.com/SAP/ui5-webcomponents/projects?type=classic

We expect to have an official release containing the working behavior at the end of June.
That will be webc v2.

Best regards,
Boyan

@unazko unazko removed the Medium Prio label Jun 6, 2024
@Abanindra
Copy link

Hi @unazko,

Since we are yet to move to v2, we are expecting the fix in one of the V 1.x. Appreciate if you can let us know by when we can expect the fix.

reagrds
Aban

@Abanindra
Copy link

Hi @unazko Can you please comment on this as we need an urgent fix for this issue?

@unazko
Copy link
Contributor

unazko commented Jul 9, 2024

Hi @Abanindra,

I've referenced a PR, which will fix this issue for 1.24 release once merged.
Currently the PR is in the review phase.

Best regards,
Boyan

unazko added a commit that referenced this issue Jul 29, 2024
@unazko
Copy link
Contributor

unazko commented Jul 29, 2024

Hi @Abanindra,

The fix is done and it is transported into 1.24 release.
The solution could be consumed with 1.24.8 patch, which is expected to be released in the beginning of August.

Best regards,
Boyan

@unazko unazko closed this as completed Jul 29, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Completed in Maintenance - Topic B Jul 29, 2024
@Abanindra
Copy link

Thank you @unazko for the update.

@rv97
Copy link
Author

rv97 commented Aug 1, 2024

Thanks @unazko !

@shubhamnazare
Copy link

Hi @unazko

I was validating the fix with the above codesandbox example and unfortunately the issue is still reproducible. The versions I used were - ui5/webcomponents: 1.24.8 and ui5/webcomponents-react: 1.29.6.

Could you please validate from your side and confirm the same if possible.

Thanks a lot!

dominiksta added a commit to dominiksta/wournal that referenced this issue Aug 30, 2024
@shubhamnazare
Copy link

Hi @unazko

I was validating the fix with the above codesandbox example and unfortunately the issue is still reproducible. The versions I used were - ui5/webcomponents: 1.24.8 and ui5/webcomponents-react: 1.29.6.

Could you please validate from your side and confirm the same if possible.

Thanks a lot!

Hi @unazko

As I mentioned this issue is still reproducible in 'ui5/webcomponents: 1.24.8' and 'ui5/webcomponents-react: 1.29.6'. Could you please validate and let me know what are the next steps?

@unazko
Copy link
Contributor

unazko commented Sep 16, 2024

Hi @shubhamnazare,

The previous sample from the codesandbox doesn't load for me.
I've made similar sample into a different platform: https://stackblitz.com/edit/vitejs-vite-pekkod?file=package.json
Just wait for the dependencies to get installed.
I couldn't reproduce the issue at my side with 1.24.8 patch.

The "New file" menu item has sub-menu and afterwards we have second level of nested sub-menus.
I've tested on Firefox, Chrome and Edge browser and also on macos and windows.
Everything works at my side.

Could you please test at your side and edit the sample if necessary so the issue is reproducible?

@rv97
Copy link
Author

rv97 commented Sep 17, 2024

Hi @unazko ,

I opened the sandbox link which you had mentioned. I am still able to reproduce the issue.
Here is the video of me doing the same: https://sap-my.sharepoint.com/:v:/p/vignesh_r02/EahKUzIpMblNsW5dGYVsaggByZ9iCPOjl1KdAwurmy2Elg?nav=eyJyZWZlcnJhbEluZm8iOnsicmVmZXJyYWxBcHAiOiJPbmVEcml2ZUZvckJ1c2luZXNzIiwicmVmZXJyYWxBcHBQbGF0Zm9ybSI6IldlYiIsInJlZmVycmFsTW9kZSI6InZpZXciLCJyZWZlcnJhbFZpZXciOiJNeUZpbGVzTGlua0NvcHkifX0&e=DoriS7

Steps:
Show Menu -> New File -> Open

You can see it is stuck at the loading indicator forever until i go back one level and open it again.

CC @shubhamnazare @Abanindra

@unazko unazko moved this from Completed to In Progress in Maintenance - Topic B Sep 17, 2024
@unazko unazko reopened this Sep 17, 2024
@unazko
Copy link
Contributor

unazko commented Sep 17, 2024

Hi @rv97,

Thanks for the reproduction steps.
I've managed to reproduce the issue again.
It seems as an additional scenario to the same issue. Hopefully I'll manage to fix it faster.
We'll be aiming for a next release next week or the week after.

@unazko
Copy link
Contributor

unazko commented Sep 20, 2024

Hi @rv97, @shubhamnazare, @Abanindra,

The issue is now fixed via the related change in 1.24.10 patch, which was just released.
I've switched to 1.24.10 patch in the sample application and the issue isn't reproducible any longer.
Please do perform a re-test at your side:
https://stackblitz.com/edit/vitejs-vite-pekkod?file=package.json

@unazko unazko closed this as completed Sep 20, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Completed in Maintenance - Topic B Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug in the code High Prio TOPIC B
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

5 participants