konveyor / tackle2-ui

Tackle (2nd generation) UI component.
Apache License 2.0
8 stars 43 forks source link

:sparkles: Update business services table to use ActionsColumn #1921

Closed mguetta1 closed 5 months ago

mguetta1 commented 6 months ago

Related to #1318

image

OR

image

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 42.05%. Comparing base (b654645) to head (a034768). Report is 172 commits behind head on main.

:exclamation: Current head a034768 differs from pull request most recent head 8d03157

Please upload reports for the commit 8d03157 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1921 +/- ## ========================================== + Coverage 39.20% 42.05% +2.85% ========================================== Files 146 166 +20 Lines 4857 5340 +483 Branches 1164 1298 +134 ========================================== + Hits 1904 2246 +342 - Misses 2939 3078 +139 - Partials 14 16 +2 ``` | [Flag](https://app.codecov.io/gh/konveyor/tackle2-ui/pull/1921/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=konveyor) | Coverage Δ | | |---|---|---| | [client](https://app.codecov.io/gh/konveyor/tackle2-ui/pull/1921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=konveyor) | `42.05% <ø> (+2.85%)` | :arrow_up: | | [server](https://app.codecov.io/gh/konveyor/tackle2-ui/pull/1921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=konveyor) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=konveyor#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sjd78 commented 5 months ago

@mguetta1 - I like the one with the edit pencil. Which option do you want to use?

ibolton336 commented 5 months ago

@mguetta1 @ibolton336 AppTableActionButtons is a wrapper that provides RBAC access check. Please provide some more context why we need to remove it.

The idea initially was to standardize on a common approach in the table now that this new table component was available. Modifying the AppTableActionButtons component to make use of the ActionsColumn may be a better approach. The use of PF Flex inside the AppTableActionButtons causes some inconsistent spacing when compared to the tables using ActionsColumn.

sjd78 commented 5 months ago

@mguetta1 @ibolton336 AppTableActionButtons is a wrapper that provides RBAC access check. Please provide some more context why we need to remove it.

The idea initially was to standardize on a common approach in the table now that this new table component was available. Modifying the AppTableActionButtons component to make use of the ActionsColumn may be a better approach. The use of PF Flex inside the AppTableActionButtons causes some inconsistent spacing when compared to the tables using ActionsColumn.

Structurally I like the "new" style for the component layout. However, with the different containing component, are we dropping some RBAC checks? That could be ok so long as there is a new effort soon (i.e. in the release after 0.5) to refactor RBAC and apply it on all actions. Humm, I may need to write up an issue to capture that work (and keycloak lib upgrades).

mguetta1 commented 5 months ago

@mguetta1 - I like the one with the edit pencil. Which option do you want to use?

This option

mguetta1 commented 5 months ago

@rszwajko @ibolton336 @sjd78 I don't think that there is any permission restrictions on controls. Anyway I added the controlsWriteScopes to the business service create button 69ff6b2

ibolton336 commented 5 months ago

@rszwajko @ibolton336 @sjd78 I don't think that there is any permission restrictions on controls. Anyway I added the controlsWriteScopes to the business service create button 69ff6b2

I think it'd be worth trying to save the component for reuse as Radek mentioned. That way we don't need a new PR for each place we make these changes & can avoid maintenance headaches.

mguetta1 commented 5 months ago

https://github.com/konveyor/tackle2-ui/assets/60384172/14457d94-b95c-4f36-a75e-a8d483077e15