feat: DataTable selections clean up and improvements #1804

adamstankiewicz · 2022-11-29T03:30:18Z

Description

Refactored to be more explicit about how often DataTable calls fetchData as well as what data it passes in its arguments.
Make children optional for Card.Section and Card.Footer.
Implements a loading state for CardView; consumer may override the skeleton card component to use with the SkeletonCardComponent prop to CardView.
Exposes a new onSelectedRowsChanged prop to DataTable as a way to inform parent/consuming components when DataTable selections has changed in some way (e.g., a row was selected/unselected).
- This approach is similar to this official code example for "controlled" selections.
Ensures TableFooter adheres to whether RowStatus is overridden by the RowStatusComponent prop on DataTable.

Deploy Preview

https://deploy-preview-1804--paragon-openedx.netlify.app/components/card/
https://deploy-preview-1804--paragon-openedx.netlify.app/components/datatable/

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

netlify · 2022-11-29T03:30:23Z

✅ Deploy Preview for paragon-openedx ready!

Name	Link
🔨 Latest commit	`4a4a696`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/6387be8cd9fd8200080935f2
😎 Deploy Preview	https://deploy-preview-1804--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

adamstankiewicz · 2022-11-29T03:31:59Z

src/Card/CardFooter.jsx

@@ -42,7 +42,7 @@ const CardFooter = React.forwardRef(({

 CardFooter.propTypes = {
  /** Specifies contents of the component. */
-  children: PropTypes.node.isRequired,
+  children: PropTypes.node,


turns out it is a valid use case to use Card.Footer without children

adamstankiewicz · 2022-11-29T03:32:10Z

src/Card/CardSection.jsx

@@ -45,7 +45,7 @@ CardSection.propTypes = {
  /** Specifies class name to append to the base element. */
  className: PropTypes.string,
  /** Specifies contents of the component. */
-  children: PropTypes.node.isRequired,
+  children: PropTypes.node,


turns out it is a valid use case to use Card.Section without children

Love the built in spacers of the Card.Section :)

adamstankiewicz · 2022-11-29T03:34:47Z

src/DataTable/CardView.jsx

+        className={classNames('pgn__data-table-card-view', className)}
+        columnSizes={columnSizes}
+      >
+        {[...new Array(8)].map(() => <SkeletonCardComponent key={nanoid()} />)}


note: might consider allowing consumers to override the number of loading skeleton cards to display

adamstankiewicz · 2022-11-29T03:35:51Z

src/DataTable/DataTable.scss

-      [dir="ltr"] & {
-        margin-right: map_get($spacers, 2);


[clarification] This style wasn't applying in MFEs since the dev build doesn't compile SCSS/CSS with the same PostCSS RTL plugin as production builds.

adamstankiewicz · 2022-11-29T03:36:25Z

src/DataTable/TableFooter.jsx

+        <>
+          <RowStatus />
+          <TablePagination />
+          <TablePaginationMinimal />
+        </>


moved from default props to be explicit in the component

adamstankiewicz · 2022-11-29T03:37:16Z

src/DataTable/hooks.jsx

@@ -27,14 +26,15 @@ export const useSelectionActions = (
 ) => {
  const [{ selectedRows, isEntireTableSelected }, dispatch] = controlledTableSelections;

-  const clearSelection = () => {
+  const clearSelection = useCallback(() => {


This exported function should be memoized so it's not responsible for any erroneous component re-renders in consuming components.

adamstankiewicz · 2022-11-29T03:38:37Z

src/DataTable/index.jsx

+  const {
+    pageSize: tableStatePageSize,
+    pageIndex: tableStatePageIndex,
+    sortBy: tableStateSortBy,
+    filters: tableStateFilters,
+    selectedRowIds: tableStateSelectedRowIds,
+  } = instance.state;


These data are the same fields as were previously passed as arguments to fetchData below, so it is not a breaking change from that perspective. This change was necessary to prevent the useEffect that calls fetchData below from calling when selections were being made and allows us to move away from the more risky/hacky delete approach in the previous implementation.

Also note, the original "controlled" selections implementation co-exists with this new approach. Need to sanity check that other uses of DataTable selections relying on the original selections implementations won't break as a result of this change.

Long term, we'll probably want to move away from or otherwise deprecate the current selections strategy, where "controlled" selections are not really controlled.

adamstankiewicz · 2022-11-29T03:39:08Z

src/DataTable/index.jsx

+
+  useEffect(() => {
+    if (onSelectedRowsChanged) {
+      onSelectedRowsChanged(tableStateSelectedRowIds);


Inform the parent/consuming component that row selections were changed in some way.

adamstankiewicz · 2022-11-29T03:42:15Z

src/DataTable/index.jsx

@@ -134,6 +148,7 @@ function DataTable({
    disableElevation,
    isLoading,
    isSelectable,
+    manualSelectColumn,


This needed to be exposed in DataTableContext for it to be accessible by the CardView.

adamstankiewicz · 2022-11-29T03:44:12Z

src/Skeleton/Skeleton.scss

@@ -0,0 +1,3 @@
+.react-loading-skeleton {
+    z-index: unset;


react-loading-skeleton puts z-index: 1 on their skeletons. This negatively impacted the shadow layering within a FullscreenModal and Stepper. Unsetting removes the styling from react-loading-skeleton.

codecov · 2022-11-29T05:06:18Z

Codecov Report

Base: 90.74% // Head: 90.77% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (4a4a696) compared to base (c02912f).
Patch coverage: 94.44% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1804      +/-   ##
==========================================
+ Coverage   90.74%   90.77%   +0.03%     
==========================================
  Files         212      212              
  Lines        3726     3739      +13     
  Branches      876      882       +6     
==========================================
+ Hits         3381     3394      +13     
  Misses        343      343              
  Partials        2        2

Impacted Files	Coverage Δ
src/Card/CardFooter.jsx	`100.00% <ø> (ø)`
src/Card/CardSection.jsx	`100.00% <ø> (ø)`
src/DataTable/SmartStatus.jsx	`100.00% <ø> (ø)`
src/DataTable/hooks.jsx	`100.00% <ø> (ø)`
src/DataTable/utils/getVisibleColumns.jsx	`100.00% <ø> (ø)`
src/DataTable/index.jsx	`95.00% <80.00%> (+0.17%)`	⬆️
src/DataTable/CardView.jsx	`100.00% <100.00%> (ø)`
src/DataTable/TableFooter.jsx	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…eight

brobro10000 · 2022-11-30T21:26:11Z

src/DataTable/CardView.jsx

  // use the same component for card selection that is used for row selection
  // otherwise view switching might break if row selection uses component that supports backend filtering / sorting
-  const selectionComponent = manualSelectColumn?.Cell || selectColumn.Cell;
+  const SelectionComponent = manualSelectColumn?.Cell || selectColumn.Cell;


[curious]
Would rewriting the logic as const SelectionComponent = manualSelectColumn?.Cell || selectColumn?.Cell; prevent non-loading page instances if selectColumn happens to be undefined?

@brobro10000 The selectColumn variable (defined in getVisibleColumns) is a constant, so it'll always exist :)

brobro10000 · 2022-11-30T21:28:10Z

src/Card/CardSection.jsx

@@ -45,7 +45,7 @@ CardSection.propTypes = {
  /** Specifies class name to append to the base element. */
  className: PropTypes.string,
  /** Specifies contents of the component. */
-  children: PropTypes.node.isRequired,
+  children: PropTypes.node,


Love the built in spacers of the Card.Section :)

edx-semantic-release · 2022-11-30T22:25:51Z

🎉 This PR is included in version 20.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

edx-semantic-release · 2023-02-17T13:28:13Z

🎉 This PR is included in version 21.0.0-alpha.12 🎉

The release is available on:

Your semantic-release bot 📦🚀

adamstankiewicz commented Nov 29, 2022

View reviewed changes

adamstankiewicz force-pushed the ags/card-datatable-updates branch from e4ce442 to c2ef781 Compare November 29, 2022 03:46

adamstankiewicz mentioned this pull request Nov 29, 2022

feat: content highlights stepper updates, including refactor to use context selectors and connects to Algolia for course search results openedx/frontend-app-admin-portal#895

Merged

3 tasks

adamstankiewicz marked this pull request as ready for review November 29, 2022 18:42

adamstankiewicz added 7 commits November 30, 2022 13:23

feat: DataTable selections clean up and improvements

e542962

chore: stylint

5778794

docs: replace unsplash with picsum

a8383fb

fix: remove 100% height on card

4b92954

fix: add height: 100% to Cards in DataTable CardView to fill column h…

cbc3428

…eight

fix: remove useCallback from clearSelection

5134398

fix: ensure RowStatus may be overriden by RowStatusComponent, if passed

7bb44c3

adamstankiewicz force-pushed the ags/card-datatable-updates branch from e12c438 to 7bb44c3 Compare November 30, 2022 18:23

fix: tests for new DataTable functionality

4a4a696

brobro10000 approved these changes Nov 30, 2022

View reviewed changes

adamstankiewicz merged commit eac3925 into master Nov 30, 2022

adamstankiewicz deleted the ags/card-datatable-updates branch November 30, 2022 22:12

edx-semantic-release added the released label Nov 30, 2022

edx-semantic-release added the released on @alpha label Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: DataTable selections clean up and improvements #1804

feat: DataTable selections clean up and improvements #1804

adamstankiewicz commented Nov 29, 2022 •

edited

Loading

netlify bot commented Nov 29, 2022 •

edited

Loading

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

brobro10000 Nov 30, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

adamstankiewicz Nov 29, 2022

codecov bot commented Nov 29, 2022 •

edited

Loading

brobro10000 Nov 30, 2022

adamstankiewicz Nov 30, 2022

brobro10000 Nov 30, 2022

edx-semantic-release commented Nov 30, 2022

edx-semantic-release commented Feb 17, 2023

feat: DataTable selections clean up and improvements #1804

feat: DataTable selections clean up and improvements #1804

Conversation

adamstankiewicz commented Nov 29, 2022 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

netlify bot commented Nov 29, 2022 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

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

Choose a reason for hiding this comment

codecov bot commented Nov 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-semantic-release commented Nov 30, 2022

edx-semantic-release commented Feb 17, 2023

adamstankiewicz commented Nov 29, 2022 •

edited

Loading

netlify bot commented Nov 29, 2022 •

edited

Loading

codecov bot commented Nov 29, 2022 •

edited

Loading