feat(TitleColumn): RHINENG-3112 - Allow passing in a link via href prop #2087

bastilian · 2023-11-10T08:35:29Z

This changes the a tag used to a proper react router Link component and allows entities to have a href prop that the Inventory should link the display_name (example in compliance[0]).

[0] https://github.com/RedHatInsights/compliance-frontend/pull/2012/files#diff-09ae52efeffb58d25e728921c02f75f65b9f249855e9eea936ce9e913d6997e1R145

bastilian · 2023-11-10T13:19:14Z

src/components/InventoryTabs/ConventionalSystems/ConventionalSystemsTab.js

@@ -295,7 +295,6 @@ const ConventionalSystemsTab = ({
          ],
        }}
        bulkSelect={bulkSelectConfig}
-        onRowClick={(_e, id, app) => navigate(`/${id}${app ? `/${app}` : ''}`)}


I'm not sure what this was meant to link to exactly. When the onRowClick is called it will receive an event, id, but the third argument is not an "app" or anything, but isMetaKey which determines if a certain keyboard key is pressed.

Also, by default the inventory table will link/navigate to the systems details page in the inventory, like it did here in the passed in onRowClick. So this became obsolete.

bastilian · 2023-11-10T13:23:20Z

src/components/ImmutableDevices/ImmutableDevices.js

@@ -40,10 +38,6 @@ const ImmutableDevices = ({
    return [...mergeAppColumns(filteredColumns), ...edgeColumns];
  };

-  const onRowClick = (_key, systemId) => {
-    navigate(`/insights/inventory/${systemId}?appName=vulnerabilities`);


This became obsolete with using the Link in the TitleColumn and the react router oddnesses being resolved, not requiring to pass in a navigate ensuring it is referencing the correct router to navigate.

bastilian · 2023-11-10T13:23:34Z

src/components/InventoryTable/EntityTable.js

@@ -70,14 +67,6 @@ const EntityTable = ({
    [loaded, columns, hasItems, rows, isExpandable]
  );

-  const defaultRowClick = (_event, key) => {


Same as above.

bastilian · 2023-11-10T13:26:18Z

src/components/InventoryTable/TitleColumn.js

  <div className="ins-composed-col sentry-mask data-hj-suppress">
-    <div key="os_release">{item?.os_release}</div>
+    {item?.os_release && <div key="os_release">{item?.os_release}</div>}


I don't think that we are showing/using this anywhere, or?

I'm not 100% sure what it even does. Would it provide another div in the title column with the os version along with the system name? If so, then I've never seen that.

Yeah, it would do just that. I'll leave it in for now, but I think we can eventually remove it.

codecov-commenter · 2023-11-13T11:59:40Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (53ebfa8) 59.20% compared to head (b224d5e) 59.21%.

Files	Patch %	Lines
src/components/InventoryTable/helpers.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2087   +/-   ##
=======================================
  Coverage   59.20%   59.21%           
=======================================
  Files         188      188           
  Lines        5944     5938    -6     
  Branches     1669     1669           
=======================================
- Hits         3519     3516    -3     
+ Misses       2425     2422    -3

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

bastilian · 2023-11-13T12:36:33Z

src/components/ImmutableDevices/ImmutableDevices.cy.js

-      .find('.ins-composed-col > :nth-child(2) > a')
-      .trigger('click');
+      .find('.ins-composed-col > div > a')
+      .click();


The click() was what fixed the failing Cypress test, which is odd, because I would expect trigger('click') to be the same and click would just be a shorthand.

bastilian · 2023-11-15T12:23:30Z

/retest

gkarat

LGTM! Have tested it locally for preview and stable routes and didn't find any issues while navigating. Also thanks for updating the tests

gkarat · 2023-11-23T12:06:02Z

src/components/InventoryTable/InventoryTable.js

@@ -343,7 +341,6 @@ InventoryTable.propTypes = {
  tableProps: PropTypes.object,
  isRbacEnabled: PropTypes.bool,
  hasCheckbox: PropTypes.bool,
-  onRowClick: PropTypes.func,


nit-pick: Since the prop is sent down to EntityTable eventually, I think it makes sense to leave it specified in propTypes here as an indication that InventoryTable generally supports this property.

InsightsLink is not fully working in scenarios where the Link is in another applications.

gkarat · 2023-11-23T16:38:11Z

🎉 This PR is included in version 1.59.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

bastilian force-pushed the title_column_href branch from 4300403 to 54d0014 Compare November 10, 2023 13:08

mkholjuraev mentioned this pull request Nov 10, 2023

chore(ImmutableDevices): allow passing onRowClick function #2080

Closed

bastilian commented Nov 10, 2023

View reviewed changes

bastilian force-pushed the title_column_href branch 2 times, most recently from 778c36d to 3a578b3 Compare November 13, 2023 11:38

bastilian marked this pull request as ready for review November 13, 2023 11:43

bastilian requested a review from a team as a code owner November 13, 2023 11:43

bastilian commented Nov 13, 2023

View reviewed changes

bastilian force-pushed the title_column_href branch from 0c0babf to 2f94129 Compare November 15, 2023 10:46

gkarat assigned bastilian Nov 21, 2023

gkarat added the enhancement New feature or request label Nov 21, 2023

bastilian force-pushed the title_column_href branch 2 times, most recently from ee10d0a to 60bb7eb Compare November 23, 2023 10:51

gkarat approved these changes Nov 23, 2023

View reviewed changes

bastilian force-pushed the title_column_href branch from 60bb7eb to b224d5e Compare November 23, 2023 13:54

bastilian added 2 commits November 23, 2023 16:25

feat(TitleColumn): RHINENG-3112 - Allow passing in a link via href prop

228d183

fix(TitleColumn): Use react router Link

0664aba

InsightsLink is not fully working in scenarios where the Link is in another applications.

bastilian force-pushed the title_column_href branch from b224d5e to 0664aba Compare November 23, 2023 15:25

bastilian merged commit f4a9b0d into RedHatInsights:master Nov 23, 2023
2 checks passed

gkarat added the released label Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(TitleColumn): RHINENG-3112 - Allow passing in a link via href prop #2087

feat(TitleColumn): RHINENG-3112 - Allow passing in a link via href prop #2087

bastilian commented Nov 10, 2023 •

edited

Loading

bastilian Nov 10, 2023

bastilian Nov 10, 2023

bastilian Nov 10, 2023

bastilian Nov 10, 2023

bastilian Nov 10, 2023

johnsonm325 Nov 14, 2023

bastilian Nov 15, 2023

codecov-commenter commented Nov 13, 2023 •

edited

Loading

bastilian Nov 13, 2023

bastilian commented Nov 15, 2023

gkarat left a comment

gkarat Nov 23, 2023

gkarat commented Nov 23, 2023

feat(TitleColumn): RHINENG-3112 - Allow passing in a link via href prop #2087

feat(TitleColumn): RHINENG-3112 - Allow passing in a link via href prop #2087

Conversation

bastilian commented Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

bastilian commented Nov 15, 2023

gkarat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkarat commented Nov 23, 2023

bastilian commented Nov 10, 2023 •

edited

Loading

codecov-commenter commented Nov 13, 2023 •

edited

Loading