🐛 Adjust Dropdown for deprecated table support in PF V5

[gildub]

Disable properly the row delete option (dropdown) when an application is involved in migration wave.

Note: doesn't need to be back-ported

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (dea3ac8) 44.10% compared to head (aed3563) 44.10%.

❗ Current head aed3563 differs from pull request most recent head 5d75d74. Consider uploading reports for the commit 5d75d74 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1154   +/-   ##
=======================================
  Coverage   44.10%   44.10%           
=======================================
  Files         177      177           
  Lines        4501     4501           
  Branches     1007     1007           
=======================================
  Hits         1985     1985           
  Misses       2505     2505           
  Partials       11       11           

Flag	Coverage Δ
unitests	44.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mturley]

@gildub I'm not sure I understand the purpose of this PR, didn't we just realize we do need the isAriaDisabled prop and that's why we are keeping the deprecated Dropdown for now? Testing this branch, the tooltip does not work (where it does without this change)

[mturley]

Oh sorry, I was testing the bulk dropdown and not the table row one. Hmm, this does break the tooltip but it fixes the disabled behavior. We need to figure out how to keep both -- maybe we need to custom render a KebabDropdown in a cell on each row instead of using the built in actions array? I suppose that is a big refactor.

[gildub]

@mturley , in which case might want to look at migrating applications pages away from the deprecated tables.
The new (composable) table giving us more flexibility especially for those complicated cases.
Although that's even a larger refactor but we might be better on that approach on the long run.
What do you think ?

[mturley]

Yeah that is definitely something we should do eventually but is probably a lot of work and should be done carefully. I would be in support though if we decided as a team that there is a good time to do it

[mturley]

Ideally we can find a way to fix this issue in the short term before we go for the full refactor

[gildub]

@mturley,
I'm trying to understand why the tooltip is now broken.
The IAction type we use for the action array is coming from "@patternfly/react-table" where the Table used (by AppTableWithControls -> AppTable) is from "@patternfly/react-table/deprecated".

Even though the implementation of IAction hasn't changed it seems the integration of the two implies a different behavior.
If that's the case I don't see how we can make that work as there is no "deprecated" version of IAction

A workaround is to force Tooltip by using isVisible: true.
The Tooltip appears as soon as the kebab is displayed:

[mturley]

Yeah, I don't think that workaround will work, we want to retain the hover behavior. I think the reason this happened is because the legacy/deprecated Table in PF was refactored to render the composable Table components under the hood, so that it was easier to maintain when there were style changes to table elements. My guess is that this shared rendering code was updated to use the new dropdown components (or something like that).

The solution that comes to mind for me is to stop using the IAction objects and instead render our own column that uses our KebabDropdown in the title property of its cells. That will be a bit of a refactor but it won't be throwaway work because we'll need the dropdown to be in that format when we eventually refactor the whole table to use the composable components.

[mturley]

I would be happy to take a crack at that if you want me to @gildub

[gildub]

@mturley, in the meantime I got that tracker for the issue about tooltip support on menu for legacy dropdown: patternfly/patternfly-react#9285

[gildub]

@mturley, to follow-up and answer you, I'm trying to see if PF fix is going to be addressing the issue soon enough so we don't have to refactor. In case they don't then we'll do as you described, use our own column and dropdown.

[gildub]

@mturley, good news, PF team is working on the issue and are hoping to fix it before PF GA.

[mturley]

Awesome! maybe we can revert my reverting of the upgrade to the new dropdowns in KebabDropdown after that then 😆

[mturley]

LGTM, but deleting and recreating the package-lock file resulted in locking new versions of dependencies unrelated to this fix. Ideally we should keep those to a dedicated PR if we want to bump them. You can solve this by reverting to the prior package-lock version:

git checkout main package-lock.json

and then updating only PF to the newest version matching the semver range in package.json using npm update:

npm update @patternfly/react-core

[mturley]

LGTM!