search

SAP / ui5-webcomponents-react

A wrapper implementation for React of the UI5 Web Components that are compliant with the SAP Fiori User Experience
https://sap.github.io/ui5-webcomponents-react/
Apache License 2.0
446 stars 101 forks source link

[Legacy Table & Popover]: [Legacy Table style broken, and popover not show beside right opener] #6553

Open JackieWei opened 4 weeks ago

JackieWei commented 4 weeks ago

Describe the bug

A popover with a opener, it shows at wrong position(opener is part of Shellbar), and also the legacy table style in popover broken too

Isolated Example

https://stackblitz.com/edit/github-wmf1wn-qspqvb?file=src%2FApp.tsx

Reproduction steps

  1. Goto example, and click avatar to open popover
  2. See that popover is open, but not beside the opener, which is the avatar
  3. See the legacy table in popover with style broken, not a table looking and feel ...

Expected Behaviour

Popover should show at bottom of avatar Legacy table in popover should work with correct style

Screenshots or Videos

No response

UI5 Web Components for React Version

~2.3.0

UI5 Web Components Version

~2.3.0

Browser

Chrome

Operating System

Mac OS

Additional Context

No response

Relevant log output

No response

Organization

No response

Declaration

Lukas742 commented 4 weeks ago

Hi @JackieWei

calling showPopover on the element is not supported; please use the open property/attribute instead. You can find more information in our documentation.

Example:

        onProfileClick={(event) => {
          if (profileMenuRef?.current) {
            profileMenuRef.current.opener = event.detail.targetRef;
            profileMenuRef.current.open = true;
          }
        }}

The issue regarding the legacy table is similar to this one: https://github.com/SAP/ui5-webcomponents-react/issues/6344

To summarize, the reason for this issue is that different custom elements with the same name (e.g. ui5-table) are defined in the same app, which is not supported. In production environments, tree shaking should do the job, but unfortunately, tree shaking capabilities are very limited in development mode for most bundlers.

We couldn't find a solution for it on our end, even by adjusting framework configurations for Vite, Next, etc., or using plugins. Of all the bundlers we tested, only Turbopack seems capable of tree shaking in development mode.

This is therefore not a small matter but requires a discussion with the UI5 Web Components team. Hopefully, together we can find a solution.

azhu716 commented 3 weeks ago

Hi @Lukas742

I tried the following code: https://stackblitz.com/edit/github-wmf1wn-dyxxbv?file=src%2Fmain.tsx And the table is still broken.

So basically if we use ANY import from ui5/webcomponents-react, the legay table won't work (at least in dev env without treeshaking) am I understanding correctly?

I'm also aware of the other issue with ui5/webcomponents team: https://github.com/SAP/ui5-webcomponents/issues/9920 and they suggest that we should not use V1 and V2 table together. However, the use of the V2 table is mandatory though not intentional, as long as we use webcomponents-react (if the above code is correct).

Please review the stackblitz above and confirm my understanding... And maybe we could think of a solution. Thanks.

azhu716 commented 3 weeks ago

By the way, we insist on using the legacy table because of the TableGroupRow. Unfortunately, that component has been deleted from the V2 table.

Lukas742 commented 3 weeks ago

Hi @azhu716

I tried the following code: https://stackblitz.com/edit/github-wmf1wn-dyxxbv?file=src%2Fmain.tsx And the table is still broken.

For me, this example using the v2 table is working as intended. As App.tsx is not imported, the v1 table isn't imported neither, so the custom element names should've been defined only by the v2 table. Am I missing something here?

So basically if we use ANY import from ui5/webcomponents-react, the legay table won't work (at least in dev env without treeshaking) am I understanding correctly?

Currently yes, but once this commit has been released, only using file-imports (import {Button} from "@ui5/webcomponents-react/dist/webComponents/Button/index.js") instead of root-imports (import {Button} from "@ui5/webcomponents-react") will also work. Please note that this should only be used as a workaround, as we usually recommend using only the root-imports.

Example using a snapshot version of ui5wcr:

https://stackblitz.com/edit/github-wmf1wn-cswuuk?file=src%2FApp.tsx,src%2Fmain.tsx

Additionally, I believe there’s a better workaround for this. The issue is caused by the order in which custom elements are defined, which depends on the order of component imports in your app. Importing the v1 table web component before any other component imports ensures that only the v1 table is defined.

Example using the latest stable version of ui5wcr:

https://stackblitz.com/edit/github-wmf1wn-3zxsvn?file=src%2FApp.tsx,src%2Fmain.tsx

I'm also aware of the other issue with ui5/webcomponents team: https://github.com/SAP/ui5-webcomponents/issues/9920 and they suggest that we should not use V1 and V2 table together. However, the use of the V2 table is mandatory though not intentional, as long as we use webcomponents-react (if the above code is correct).

Unfortunately, this is the core issue here. We didn’t account for the fact that tree shaking isn’t enabled for most bundlers in dev mode, which results in all components being imported, including the custom element definitions.

By the way, we insist on using the legacy table because of the TableGroupRow. Unfortunately, that component has been deleted from the V2 table.

If this missing feature is a blocker for you, please consider opening a feature request in the ui5-webcomponents repo for the v2 table.

azhu716 commented 3 weeks ago

Hi @Lukas742

Sorry, I made some unintentional changes to the stackblitz... I was trying to emphasize that even if we only import the V1 table, it won't work. Now I fixed the sample: https://stackblitz.com/edit/github-wmf1wn-dyxxbv?file=src%2Fmain.tsx

After our evaluation, we think that the effort of customizing the TableGroupRow is acceptable. So we decided to migrate to the V2 table and mitigate the changes.

For now, we are fine to close this issue. However, there is unfortunately no way to use the V1 table via @ui5/webcomponents-react-compat. I would suggest completely dropping the V1 table support unless a solution is provided from the ui5-webcomponents side. Otherwise, it will mislead the library consumers.

cc @JackieWei

Lukas742 commented 3 weeks ago

Hi @azhu716

thanks for your feedback! We'll keep the issue open until we decided how to proceed with the problem.

Sorry, I made some unintentional changes to the stackblitz... I was trying to emphasize that even if we only import the V1 table, it won't work. Now I fixed the sample: https://stackblitz.com/edit/github-wmf1wn-dyxxbv?file=src%2Fmain.tsx

This behavior is fixed by the commit I mentioned above, it's currently broken because the Table relies on a root import from @ui5/webcomponents-react which is therefore defining the v2 table elements before the v1 table. The fix will probably be released this week. Also, a general workaround is adding the respective table imports before any other import from @ui5/webcomponents-react or @ui5/webcomponents-react-charts.