feat(virtual-table): split virtualized table, cluster settings table

[priley86]

start detail table conversion in #1986 for all detail tables in Cluster Settings.

• split out detail table into a new component. Virtualized table "@patternfly/react-virtualized-extension" and "@patternfly/react-table" have different APIs due to the nature of virtualized table body. Their row definitions are different, as well as the expected markup/jsx. Also, checking virtualized everywhere in the current Table abstraction just seems wrong. This was never my original intent... had previously called the rows Vr and Vd (virtual row), but I still think this component should be split out into two components.

Table:
https://patternfly-react.surge.sh/patternfly-4/components/table/
Virtual Table:
https://patternfly-react.surge.sh/patternfly-4/virtual%20scroll/virtualized/

cc: @TheRealJon @jcaianirh @spadgett

also, filed 2045 upstream in pf-next today. Currently, the CSS is here in overrides for this. Fixed by rescoping the following styles to virtualized tables only:

.pf-c-table.pf-c-virtualized tbody > tr > :first-child::before {
  content: none;
  width: 0 !important;
}

Codepen: https://codepen.io/priley86-the-sans/pen/JQgNrW

[priley86]

updated... @spadgett PTAL

[spadgett]

cc @christianvogt @vojtechszocs

[spadgett]

Again this is probably OK, although this table wasn't virtualized before.

[spadgett]

This table was also not virtualized previously, but probably OK.

cc @jtomasek

[spadgett]

@dtaylor113 Let me know if you think this might be a problem as this table wasn't virtualized before

[dtaylor113]

This is a standard list view so probably ok.

[spadgett]

Are these table columns all equal width? How does that look? Is there a way to specify different widths?

[priley86]

this should currently match the default pf4 table behavior (w/ any CSS overrides we have for pf-c-table). I am not explicitly overriding column widths for Table at the moment though, so the default pf4 responsive behavior would apply.

https://pf4.patternfly.org/components/Table/examples/
https://pf4.patternfly.org/components/Table/examples-full/?component=Simple%20table

[spadgett]

this should currently match the default pf4 table behavior (w/ any CSS overrides we have for pf-c-table).

So equal width columns? We might look at this since many of our tables have columns with data significantly wider than other columns. Nodes page comes to mind (image name and size).

[priley86]

Please see the "Table w/ width modifiers" example:
https://pf4.patternfly.org/components/Table/examples/

.pf-m-width-[10, 15, 20, 25, 30, 35, 40, 45, 50, 60, 70, 80, or 90], that is the default functionality for PF4 table width modifiers at the moment.

The existing Bootstrap column width modifiers you have should also work similarly to PF's .pf-u-w-25{-on-[breakpoint]} modifiers, however I don't think you'd want to use mobile modifiers now that we are stacking here and using the default PF4 mobile behavior.
https://pf4.patternfly.org/utilities/Sizing/examples/ (we did not load these since they do the same thing as Bootstrap though)

We should probably file anything upstream in PF if we determine something else is needed on this.

[priley86]

so we could pass:

{ title: <CamelCaseWrap value={condition.type} />, props: { className: 'pf-m-width-50` },

or

{ title: <CamelCaseWrap value={condition.type} />, props: { className: 'col-md-6` },

for example...

[spadgett]

We're adding a fair amount of duplicate code here to make this change :/

[priley86]

hmm, yea i see this both ways. The value being, we aren't overabstracting and the API should be easier to follow and consume (and test).

[priley86]

Not sure if Redux state should be shared now...

[priley86]

FWIW, we are prepping to convert React-Table to Typescript next in PF #1950. Making some good progress w/ this locally... this should make API typings more consistent from the PF side. Will be sure to test this out here downstream before anything goes up...

[spadgett]

@priley86 If the stateToProps should be identical between these two tables, we can share the implementation as a common tableStateToProps function?

My only hesitation with this change is that we're copying this complicated chunk of code between the two tables.

[spadgett]

Ah I guess it doesn't work because it uses the props from the component. Hm

[priley86]

extracted this logic to a common tableStateToProps function

[spadgett]

/assign

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: priley86
To complete the pull request process, please assign spadgett
You can assign the PR to them by writing /assign @spadgett in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• frontend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[priley86]

• rebased

[priley86]

/test e2e-aws

[openshift-ci-robot]

@priley86: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	e3c1575	link	/test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[vojtechszocs]

FYI @jelkosz @gnehapk

[priley86]

@spadgett - let me know when you'd like this rebased again. thanks!

[openshift-ci-robot]

@priley86: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.