Add props for sort indicators, checkboxes and expand to Skeleton Table

[CodyWMitchell]

Added the following props to the SkeletonTable component (RHCLOUD-32382):

• isSelectable: A boolean prop that determines whether the table rows are selectable. When set to true, checkboxes will be rendered in a column to the left side.

• isExpandable: A boolean prop that indicates whether the table rows are expandable. When set to true, a caret expand indicator will be rendered in a column to the left side.

• sortBy: An object that specifies the sort column and direction for the table. It has the following shape:

  {
    index: number;
    direction: 'asc' | 'desc';
  }
  

  • index represents the index of the column to sort by.
  • direction indicates the sort direction, either 'asc' for ascending order or 'desc' for descending order.

[patternfly-build]

Preview: https://react-component-groups-pr-component-groups-139.surge.sh

A11y report: https://react-component-groups-pr-component-groups-139-a11y.surge.sh

[fhlavac]

@CodyWMitchell for the selectable table, we would also need the possibility to display radios instead of checkboxes - could you please add selectVariant with options checkbox (default) and radio?

[fhlavac]

Please do not remove the comments - these prop descriptions are displayed in the docs. Can you please add them also for the new props?

[fhlavac]

I'm not sure this sortBy prop is ideal. In the current state the sort indicator is displayed for all Th cells which is not what the user of our component may want to do (most often only certain columns are sortable). Also this way you are able to specify only one column which has sorting active (user can sort by multiple columns at the same time). Another thing is that the sort indicator should not be clickable while loading.

One thing I would propose (I know it is a breaking change) is maybe to get rid of the sortBy, change the columns to columns: string[] | React.ReactElement<typeof Th>[];. And edit the rendering logic to something like this:

<Table aria-label="Loading" variant={variant} ouiaId={ouiaId} {...rest}>
      {caption && <Caption>{caption}</Caption>}
      <Thead data-ouia-component-id={`${ouiaId}-thead`}>
        <Tr ouiaId={`${ouiaId}-tr-head`}>
          {isExpandable && <Th key="expand" />}
          {isSelectable && <Th key="select" />}
          {hasCustomColumns(props)
            ? (props.columns.map((c, index) => (
              typeof c === 'string' ? <Th
                key={index}
                data-ouia-component-id={`${ouiaId}-th-${index}`}
              >
                {c}
              </Th> : React.cloneElement(c, { key: index, 'data-ouia-component-id': `${ouiaId}-th-${index}` })
            )))
            : rowArray.map((_, index) => (
              <Th
                key={index}
                data-ouia-component-id={`${ouiaId}-th-${index}`}
              >
                <Skeleton />
              </Th>
            ))}
        </Tr>
      </Thead>
      <Tbody data-ouia-component-id={`${ouiaId}-tbody`}>{bodyRows}</Tbody>
    </Table>

Then you should be able to pass just an array of strings if you don't care about sorting or specify everything using the original Ths like:

<SkeletonTable rowSize={10} columns={[ <Th key="1" sort={{ columnIndex: 0, sortBy: { index: 0, direction: 'asc' } }}>foo</Th>, <Th key="2">foo</Th> ]} />

This lifts the sorting completely to the user of our component and we don't have to care about it.

Another way would be just to pass some flag indicating if given column is sortable and do not care about active filter.

What do you think?

Cc @Hyperkid123

[CodyWMitchell]

I like this approach! Added this in.

[fhlavac]

@CodyWMitchell could you please also add examples for all the options (sorting, expandable table, selection) to the docs?
Sorry for too many comments. It looks great! 🎉

[fhlavac]

Suggested change

	isSelected: false
	isSelected: false,
	isDisabled: true

Let's make the checkboxes/radios disabled

[CodyWMitchell]

Thanks for all of the great feedback @fhlavac ! 🥇

[fhlavac]

Suggested change

	<Th key="1" sort={{ columnIndex: 0, sortBy: { index: 0, direction: 'asc' } }}>
	<Th key="1" sort={{ columnIndex: 0, sortBy: {} }}>

Not sure if we want to quide people to show the sort direction while loading - it looks clickable that way and we probably don't want people to click it (potentially send another API call if one is still in progress). Maybe leaving sortBy empty would be better for an example

[fhlavac]

Linter and Jest look to be failing, otherwise, it looks great!

[CodyWMitchell]

Thanks for the review, @fhlavac! 👍 Checks should be passing now.

[fhlavac]

Looks good, I just added a commit to trigger a release

[github-actions]

🎉 This PR is included in version 5.2.0-prerelease.4 🎉

The release is available on:

• GitHub release
• npm package (@prerelease dist-tag)

Your semantic-release bot 📦🚀