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

✨ Add edit icon with ActionColumns property in the Archetypes table #2098

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

Shevijacobson
Copy link
Contributor

@Shevijacobson Shevijacobson commented Sep 16, 2024

Changes Made:

  • Removed the edit action from the ActionsColumn.

  • Added a dedicated edit icon outside the ActionsColumn, providing a clearer and more focused user interface.

Part of #1318

Before the change:

Screenshot from 2024-09-29 10-24-26

After the change:

Screenshot from 2024-09-29 10-28-03

@Shevijacobson Shevijacobson force-pushed the m_s_ar-table branch 3 times, most recently from a0d63b6 to b14dceb Compare September 18, 2024 08:19
@mguetta1 mguetta1 marked this pull request as ready for review September 18, 2024 17:22
Copy link
Collaborator

@mguetta1 mguetta1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Sep 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.97%. Comparing base (b654645) to head (97e4355).
Report is 239 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2098      +/-   ##
==========================================
+ Coverage   39.20%   41.97%   +2.77%     
==========================================
  Files         146      175      +29     
  Lines        4857     5629     +772     
  Branches     1164     1415     +251     
==========================================
+ Hits         1904     2363     +459     
- Misses       2939     3145     +206     
- Partials       14      121     +107     
Flag Coverage Δ
client 41.97% <ø> (+2.77%) ⬆️
server ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Shevijacobson <[email protected]>
Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Code looks good! 2 minor comments:

  1. it would be nice to rephrase PR title to better reflect the change - it will be used as git commit title and stay in the repo history forever:)
  2. if the change has some visual effect it's also nice to add before/after screenshots in a separate commit comment.

@rszwajko
Copy link
Collaborator

@mguetta1
It seems we have a general problem when multiple cells with isActionCell property are displayed - in list view (small screen) they get stacked (i.e. see below). Can you open an issue to track this? I think that with upgrade to newer PatterFly version we could solve it by using OverflowMenu as shown here
image

@Shevijacobson Shevijacobson changed the title ✨ addition edit icon out of ActionColumns ✨ addition edit icon out of ActionColumns to Archetypes table Sep 29, 2024
@Shevijacobson Shevijacobson changed the title ✨ addition edit icon out of ActionColumns to Archetypes table ✨ Add edit icon with ActionColumns property in the Archetypes table Sep 29, 2024
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

LGTM

@mguetta1
Copy link
Collaborator

@mguetta1 It seems we have a general problem when multiple cells with isActionCell property are displayed - in list view (small screen) they get stacked (i.e. see below). Can you open an issue to track this? I think that with upgrade to newer PatterFly version we could solve it by using OverflowMenu as shown here image

An issue has been opened to address the above: #2111

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.

4 participants