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

🐛 Applications: bulk delete disabled when no selection #1149

Merged
merged 1 commit into from
Jul 17, 2023
Merged

🐛 Applications: bulk delete disabled when no selection #1149

merged 1 commit into from
Jul 17, 2023

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jul 17, 2023

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (74c0bc9) 44.08% compared to head (52eae0d) 44.08%.

❗ Current head 52eae0d differs from pull request most recent head 2a697f9. Consider uploading reports for the commit 2a697f9 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1149   +/-   ##
=======================================
  Coverage   44.08%   44.08%           
=======================================
  Files         177      177           
  Lines        4500     4500           
  Branches     1008     1008           
=======================================
  Hits         1984     1984           
  Misses       2505     2505           
  Partials       11       11           
Flag Coverage Δ
unitests 44.08% <ø> (ø)

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

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

@gildub gildub self-assigned this Jul 17, 2023
@gildub gildub requested a review from mturley July 17, 2023 15:20
Copy link
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

Just a minor nitpick

@@ -419,6 +419,9 @@ export const ApplicationsTableAnalyze: React.FC = () => {
importWriteAccess = checkAccess(userScopes, importsWriteScopes),
applicationWriteAccess = checkAccess(userScopes, applicationsWriteScopes);

const areAppsInWaves = () =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't need to be a function, it can just be a boolean variable. It is immediately called twice in the same render when it only needs to run the some loop once

@@ -608,6 +608,9 @@ export const ApplicationsTable: React.FC = () => {
importWriteAccess = checkAccess(userScopes, importsWriteScopes),
applicationWriteAccess = checkAccess(userScopes, applicationsWriteScopes);

const areAppsInWaves = () =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

gildub pushed a commit that referenced this pull request Jul 17, 2023
…ecated `DropdownItems`, use `isAriaDisabled` prop for items with tooltips (#1150)

Fixes a regression caused by #1126 where dropdown items that are
disabled with tooltips could not trigger the tooltips. Discovered by
@gildub when working on #1149.

It appears the `isAriaDisabled` prop which we need for retaining the
mouse hover event on a disabled DropdownItem is not present in the new
PF5 DropdownItem. This is due to missing styles in PF core that are
still being addressed (see
patternfly/patternfly#5528,
patternfly/patternfly#5692, and related comment
here:
patternfly/patternfly-react#8992 (comment))

Until `isAriaDisabled` is supported in the new PF5 DropdownItem we must
restore the usage of the deprecated component for these tooltips to
work.
Copy link
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

2 participants