Fixes #30432 - Add pf4 table component #7829

johnpmitsch · 2020-07-17T15:08:48Z

This is the pattternfly 4 table that we have been using in Katello, I'm moving it here.

The goal of this table is to make a non-opinionated table that provides:

Search
Pagination
Empty state handling
Error state handling
Allows passing props to the pf4 table for customization

I called it TableWrapper as I don't see this as an opinionated ForemanTable, rather the pf4 table with some batteries included. I think we can build a more opinionated ForemanTable or variations of this on top of this, but doing so now without a lot of use cases would mean a lot of speculation around the features we need.

I'm not married to the name so feel free to suggest something that conveys this better :)

Still a lot to do here, but it is functional and rendering, I was able to swap out for the local component in Katello and it worked well. There is some existing technical debt that has to be figured out here, Katello has been using a different Search component than Foreman for a while, I moved it over as well, this has been converted to pf4.

For now, you can test this out by going to the stories under pf4 > TableWrapper: npm run stories

The big remaining tasks are:

Add and update tests for TableWrapper and Search
Add more storybook examples - ~~currently I am blocked by not being able to mock the API~~
Update to use foreman's empty state
Move downshift and any other introduced dependencies to foreman-js
Fix pagination issue where 0 results still show number
More testing
Should we use Katello's search?

theforeman-bot · 2020-07-17T15:09:01Z

Issues: #30432

johnpmitsch · 2020-07-17T15:18:07Z

I just moved this from Katello without any changes for now and am opening as a draft PR to start the discussion.

I'll clean this up, adding tests and stories. It will likely also need some foreman-js changes, same as this PR too.

You can see usage in the new katello CV page https://github.com/Katello/katello/blob/master/webpack/scenes/ContentViews/Table/ContentViewsTable.js

I named it TableWrapper as the usage is similar to the original pf4 table, with pagination and search added. The rows and columns still need to be structured the same way that patternfly4 expects

The goal is to provide a table that has search and pagination that can display content from the API. I would consider this the first iteration of the table, I know there are more features and support for Rails content, but as long as we don't limit ourselves that all can be added later and iterated on.

houndci-bot · 2020-10-29T00:14:31Z

webpack/assets/javascripts/react_app/components/PF4/TypeAhead/pf4Search/TypeAheadSearch.scss

+.foreman-pf4-search-clear {
+  padding: 8px 5px 4px 5px;
+  margin-left: 10px; 
+}


Unexpected missing end-of-source newline no-missing-end-of-source-newline

houndci-bot · 2020-10-29T00:14:31Z

webpack/assets/javascripts/react_app/components/PF4/TypeAhead/pf4Search/TypeAheadSearch.scss

+// accounting for excess padding on search bar and aligning vertically
+.foreman-pf4-search-clear {
+  padding: 8px 5px 4px 5px;
+  margin-left: 10px; 


Expected newline before "}" of a multi-line block block-closing-brace-newline-before
Unexpected whitespace at end of line no-eol-whitespace

houndci-bot · 2020-10-29T00:14:31Z

webpack/assets/javascripts/react_app/components/PF4/TypeAhead/pf4Search/TypeAheadInput.scss

+.foreman-pf4-search-input {
+  padding-right: 35px;
+  margin-right: -23px;
+}


Unexpected missing end-of-source newline no-missing-end-of-source-newline

johnpmitsch · 2020-11-13T14:55:10Z

Still a lot to do here, but it is functional and rendering, I was able to swap out for the local component in Katello and it worked well. There is some existing technical debt that has to be figured out here, Katello has been using a different Search component than Foreman for a while, I moved it over as well, this has been converted to pf4. Also, there is a new empty state page in Foreman that could make more sense to use here.

The big remaining tasks are:

Add and update tests for TableWrapper and Search

Add more storybook examples - currently I am blocked by not being able to mock the API, I tried to set something up similar to

foreman/webpack/assets/javascripts/react_app/components/hosts/storage/vmware/__tests__/StorageContainer.stories.js

Lines 8 to 18 in 53d5bc8

    
           const initializeMocks = () => { 
        
             mockRequest({ 
        
               url: '/api/v2/compute_resources/1/available_storage_domains', 
        
               response: VMWareData.storageDomainResponse, 
        
             }); 
        
             mockRequest({ 
        
               url: '/api/v2/compute_resources/1/available_storage_pods', 
        
               response: VMWareData.storagePodResponse, 
        
             }); 
        
           };

but didn't have any luck, it doesn't seem to be working for that story as well

Update to use foreman's empty state
Move downshift and any other introduced dependencies to foreman-js
Fix pagination issue where 0 results still show number

MariaAga · 2020-11-15T10:29:24Z

@johnpmitsch Your solution was almost there :)
Looks like the mock needs an exact url (without the port) so I got it to work using:

  const path = window.location.protocol + "//" + window.location.hostname
  mockRequest({
    url: `${path}/api/v2/compute_resources/1/available_storage_domains`,
    response: VMWareData.storageDomainResponse,
  });

johnpmitsch · 2020-11-16T13:48:42Z

@johnpmitsch Your solution was almost there :)
Looks like the mock needs an exact url (without the port) so I got it to work using:
  const path = window.location.protocol + "//" + window.location.hostname
  mockRequest({
    url: `${path}/api/v2/compute_resources/1/available_storage_domains`,
    response: VMWareData.storageDomainResponse,
  });

Thank you!! 🙏

houndci-bot · 2020-12-01T00:55:20Z

webpack/assets/javascripts/react_app/components/PF4/TypeAhead/pf4Search/TypeAheadSearch.scss

+.foreman-pf4-search-clear {
+  padding: 8px 5px 4px 5px;
+  margin-left: 10px;
+}


Unexpected missing end-of-source newline no-missing-end-of-source-newline

johnpmitsch · 2021-01-15T14:05:19Z

Moving to ready for review, it still needs more testing and there are some open questions, but I think it's a good time to get feedback.

MariaAga · 2021-01-19T08:19:18Z

@johnpmitsch Can this pr be broken into a few prs? Like one for search, one for pagination, one for the empty states?

jeremylenz

The storybook story isn't rendering for me. It displays an error:

TypeError: (0 , _ForemanContext.usePaginationOptions) is not a function
    at TableWrapper (http://centos7-katello-devel.redhatlaptop.example.com:6006/main.0fd139abeda772fcc60c.bundle.js:24318:62)
    at renderWithHooks (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:510743:18)
    at mountIndeterminateComponent (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:513569:13)
    at beginWork (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:514807:16)
    at HTMLUnknownElement.callCallback (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:499703:14)
    at Object.invokeGuardedCallbackDev (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:499752:16)
    at invokeGuardedCallback (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:499814:31)
    at beginWork$1 (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:519717:7)
    at performUnitOfWork (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:518529:12)
    at workLoopSync (http://centos7-katello-devel.redhatlaptop.example.com:6006/foreman-vendor.bundle-v7.2.9-development-db270870644d27fe3377.js:518460:5)

Ron-Lavi · 2021-03-08T13:58:16Z

@johnpmitsch, what is the status of this PR? :)

This adds a table component to patternfly 4 that wraps the pf4 table with the following features: - Search - Pagination - Empty state handling - allow passthrough of props to pf4 table

johnpmitsch · 2021-03-08T14:37:15Z

@jeremylenz @LaViro I rebased and fixed the pagination hook error, the storybook should be all set to test out, let me know if it gives any more trouble.

I would love to start getting some feedback, there are open questions and to-dos but everything is functional, it would be great to get feedback on the design.

johnpmitsch · 2021-03-08T14:38:13Z

@johnpmitsch Can this pr be broken into a few prs? Like one for search, one for pagination, one for the empty states?

I'll look into this!

jeremylenz

Thanks @johnpmitsch, stories are working now :)

The autocomplete is kinda wonky in the story - It just suggests "bread" all the time but doesn't suggest operators or values. And I never get any search results. Not saying you have to have a fully functional autocomplete for the storybook, but if nonfunctionality is intended it would be nice to make it more graceful somehow, or at least call out that it's not going to work the same as a real one.

Some more initial comments below

jeremylenz · 2021-03-08T14:50:24Z

webpack/assets/javascripts/react_app/components/PF4/Table/TableWrapper.stories.js

+const ForemanContext = getForemanContext();
+
+export default {
+  title: 'Components|PF4/TableWrapper',


The old hierarchy separator | should not be used any more. They should all be /

As it is, this renders as a new top-level section called "Components|PF4". If you think there should be a new top-level section for PF4 components, I'd be fine with that, but it's too confusing to use |; I'd use "PF4 Components" or something.

jeremylenz · 2021-03-08T15:04:01Z

webpack/assets/javascripts/react_app/components/PF4/TypeAhead/pf4Search/TypeAheadSearch.js

@@ -0,0 +1,85 @@
+import React from 'react';


Would like to see a story for this component!

jeremylenz · 2021-03-08T15:05:16Z

webpack/assets/javascripts/react_app/nockWrapper.js

+// library. This is helpful when the request is not coming from Katello. For example, axios
+// called within Katello can be mocked with axios-mock-adapter or similar, but a http request
+// made by axios that is coming from Foreman cannot be mocked by axios-mock-adapter or a
+// jest mock within Katello. So to do this, we can mock the request a level deeper within


please rephrase so it's not Katello-specific :)

jeremylenz · 2021-03-08T15:10:21Z

webpack/assets/javascripts/react_app/components/PF4/Table/TableWrapper.js

+        <FlexItem align={{ default: 'alignRight' }}>
+          <Pagination


I'm thinking that the location of <Pagination> should maybe be more flexible. We've already been requested to put <Pagination> in various places, (on the left, outside the table, etc.) Would it be better to just not include it here, and allow the consumer to insert it as one of the <FlexItem> children?

jeremylenz · 2021-03-08T15:13:00Z

webpack/assets/javascripts/react_app/components/PF4/Table/MainTable.js

+}) => {
+  const isFiltering = activeFilters || searchIsActive;
+  if (status === STATUS.PENDING) return <Loading showText={false} />;
+  // Can we display the error message?


This comment no longer makes sense without the others that used to be there

jeremylenz · 2021-03-08T15:17:19Z

webpack/assets/javascripts/react_app/components/PF4/Table/TableWrapper.js

+        </FlexItem>
+      </Flex>
+      <MainTable
+        searchIsActive={!!searchQuery}


jeremylenz · 2021-03-08T15:22:51Z

webpack/assets/javascripts/react_app/components/PF4/Table/__tests__/TableWrapper.test.js

+  // Loading icon shows first
+  expect(getByLabelText(loadingLabel)).toBeInTheDocument();
+  // Loading icon disappears
+  await patientlyWaitFor(() => expect(queryByLabelText(loadingLabel)).not.toBeInTheDocument());


TableWrapper.test.js (7.693 s)

Tests that use this helper are still taking forever. I think it's worth filing an issue to see if there's any way we can improve this.

jeremylenz · 2021-03-08T15:24:12Z

webpack/assets/javascripts/react_app/components/PF4/TypeAhead/TypeAhead.js

@@ -0,0 +1,115 @@
+import React, { Component } from 'react';
+import Downshift from 'downshift';


would be nice to see a comment here to refresh my memory on what Downshift does for us

jeremylenz · 2021-03-08T15:24:53Z

webpack/assets/javascripts/react_app/components/PF4/TypeAhead/TypeAhead.js

+
+import './TypeAhead.scss';
+
+class TypeAhead extends Component {


There's no reason this can't be written with hooks 👼

jeremylenz · 2021-03-08T15:25:38Z

webpack/assets/javascripts/react_app/components/PF4/Search/Search.js

+import api from '../../../redux/API/API';
+import { stringIncludes } from './helpers';
+
+class Search extends Component {


Same here, why not write this with hooks 👼

johnpmitsch · 2021-03-22T13:14:30Z

@LaViro has kindly offered to take this PR over in my absence, so I am going to close this out. Anyone is free to use any code from this PR.

Thanks @LaViro! 👏

theforeman-bot added Needs testing Not yet reviewed UI labels Jul 17, 2020

ezr-ondrej assigned amirfefer Jul 22, 2020

johnpmitsch mentioned this pull request Sep 22, 2020

Fixes #29997 - Add Repositories tab to new Content View page Katello/katello#8939

Merged

14 tasks

johnpmitsch force-pushed the pf4table branch from d000d82 to e9d9c0e Compare October 29, 2020 00:14

theforeman-bot added the Waiting on contributor label Oct 29, 2020

houndci-bot reviewed Oct 29, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Nov 13, 2020

johnpmitsch force-pushed the pf4table branch from e78719c to aa6c823 Compare November 16, 2020 13:39

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Nov 16, 2020

johnpmitsch force-pushed the pf4table branch from 06ed122 to 705de7d Compare December 1, 2020 00:55

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Dec 1, 2020

houndci-bot reviewed Dec 1, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Dec 2, 2020

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Dec 3, 2020

johnpmitsch force-pushed the pf4table branch from c3774d7 to a423a6c Compare December 3, 2020 15:48

theforeman-bot removed the Waiting on contributor label Dec 3, 2020

johnpmitsch marked this pull request as ready for review January 15, 2021 14:02

johnpmitsch requested a review from a team as a code owner January 15, 2021 14:02

theforeman-bot added Waiting on contributor and removed Needs testing Not yet reviewed labels Jan 15, 2021

theforeman deleted a comment from theforeman-bot Jan 15, 2021

johnpmitsch force-pushed the pf4table branch from a423a6c to 5e559b8 Compare January 15, 2021 14:06

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 15, 2021

jeremylenz reviewed Jan 20, 2021

View reviewed changes

Fixes #30432 - Add pf4 table component

5512151

This adds a table component to patternfly 4 that wraps the pf4 table with the following features: - Search - Pagination - Empty state handling - allow passthrough of props to pf4 table

johnpmitsch force-pushed the pf4table branch from 5e559b8 to 5512151 Compare March 8, 2021 14:33

jeremylenz reviewed Mar 8, 2021

View reviewed changes

johnpmitsch closed this Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30432 - Add pf4 table component #7829

Fixes #30432 - Add pf4 table component #7829

johnpmitsch commented Jul 17, 2020 •

edited

Loading

theforeman-bot commented Jul 17, 2020

johnpmitsch commented Jul 17, 2020 •

edited

Loading

houndci-bot Oct 29, 2020

houndci-bot Oct 29, 2020

houndci-bot Oct 29, 2020

johnpmitsch commented Nov 13, 2020 •

edited

Loading

MariaAga commented Nov 15, 2020

johnpmitsch commented Nov 16, 2020

houndci-bot Dec 1, 2020

johnpmitsch commented Jan 15, 2021 •

edited

Loading

MariaAga commented Jan 19, 2021

jeremylenz left a comment

Ron-Lavi commented Mar 8, 2021

johnpmitsch commented Mar 8, 2021

johnpmitsch commented Mar 8, 2021

jeremylenz left a comment

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

jeremylenz Mar 8, 2021

johnpmitsch commented Mar 22, 2021

		@@ -0,0 +1,115 @@
		import React, { Component } from 'react';
		import Downshift from 'downshift';


		import './TypeAhead.scss';

		class TypeAhead extends Component {

Fixes #30432 - Add pf4 table component #7829

Fixes #30432 - Add pf4 table component #7829

Conversation

johnpmitsch commented Jul 17, 2020 • edited Loading

theforeman-bot commented Jul 17, 2020

johnpmitsch commented Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Nov 13, 2020 • edited Loading

MariaAga commented Nov 15, 2020

johnpmitsch commented Nov 16, 2020

Choose a reason for hiding this comment

johnpmitsch commented Jan 15, 2021 • edited Loading

MariaAga commented Jan 19, 2021

jeremylenz left a comment

Choose a reason for hiding this comment

Ron-Lavi commented Mar 8, 2021

johnpmitsch commented Mar 8, 2021

johnpmitsch commented Mar 8, 2021

jeremylenz left a 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

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

johnpmitsch commented Mar 22, 2021

johnpmitsch commented Jul 17, 2020 •

edited

Loading

johnpmitsch commented Jul 17, 2020 •

edited

Loading

johnpmitsch commented Nov 13, 2020 •

edited

Loading

johnpmitsch commented Jan 15, 2021 •

edited

Loading