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

Support Tree Table in DataView #19

Merged
merged 7 commits into from
Sep 11, 2024
Merged

Support Tree Table in DataView #19

merged 7 commits into from
Sep 11, 2024

Conversation

fhlavac
Copy link
Collaborator

@fhlavac fhlavac commented Sep 5, 2024

RHCLOUD-34946

  • enhanced the DataViewTable component to support displaying data in a tree table
  • added documentation
  • enhanced tests
image

@patternfly-build
Copy link

patternfly-build commented Sep 5, 2024

@fhlavac fhlavac changed the title Init Support Tree Table in DataView Sep 5, 2024
@fhlavac fhlavac marked this pull request as ready for review September 5, 2024 21:12
Copy link
Collaborator

@radekkaluzik radekkaluzik left a comment

Choose a reason for hiding this comment

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

LGTM

<Thead data-ouia-component-id={`${ouiaId}-thead`} {...props}>
<Tr ouiaId={`${ouiaId}-tr-head`}>
{onSelect && isSelected && !isTreeTable && <Th key="row-select" screenReaderText='Data selection table header cell' />}
{columns.map((column, index) => (

Choose a reason for hiding this comment

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

I'd probably memoize this JSX.

Comment on lines 55 to 56
const areAllDescendantsSelected = (node: DataViewTrTree) => getDescendants(node).every((n) => isSelected?.(n));
const areSomeDescendantsSelected = (node: DataViewTrTree) => getDescendants(node).some((n) => isSelected?.(n));

Choose a reason for hiding this comment

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

Memo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Hyperkid123 I'm just not sure what I could use as a dependency for useMemo in these cases

Copy link

@Hyperkid123 Hyperkid123 Sep 9, 2024

Choose a reason for hiding this comment

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

Whatever changes the outcomes. The iSnodeChecked which is using these might be a better place. Or the getDescendants. There is a lot if iterations happening in general. It can be a touch more efficient.

}
};

const childRows =

Choose a reason for hiding this comment

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

memo

<Table isTreeTable aria-label="Data table" ouiaId={ouiaId} {...props}>
<DataViewTableHeader isTreeTable columns={columns} ouiaId={ouiaId} />
<Tbody>
{renderRows(rows)}

Choose a reason for hiding this comment

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

I'd store the node(s) into a variable and just use that instead of calling the function.

Comment on lines 58 to 66
const isNodeChecked = (node: DataViewTrTree) => {
if (areAllDescendantsSelected(node)) {
return true;
}
if (areSomeDescendantsSelected(node)) {
return null;
}
return false;
};

Choose a reason for hiding this comment

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

This can be done in one iteration.

@fhlavac fhlavac requested a review from Hyperkid123 September 9, 2024 16:29
@fhlavac
Copy link
Collaborator Author

fhlavac commented Sep 10, 2024

@Hyperkid123 made some cleanup. The issues should be resolved now, thanks!

);
};
export const DataViewTable: React.FC<DataViewTableProps> = ({ isTreeTable, ...props }: DataViewTableProps) => (
isTreeTable ? (<DataViewTableTree {...(props as DataViewTableTreeProps)} />) : (<DataViewTableBasic {...(props as DataViewTableBasicProps)}/>)

Choose a reason for hiding this comment

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

You can make the type condition so you won't have to type cast the props.

@fhlavac fhlavac merged commit 8c14e4c into patternfly:v5 Sep 11, 2024
7 checks passed
@fhlavac fhlavac deleted the tree branch September 11, 2024 12:15
Copy link

🎉 This PR is included in version 5.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants