Summary rows #1773

qili26 · 2019-10-07T18:04:43Z

Add summaryRows to ReactDataGrid.tsx and rows sticky container
Add Special logic for IE / IE Edge
Add isSummaryRow to Row.tsx / Cell.tsx
unit test

…branch

qili26 · 2019-10-07T18:05:02Z

examples/assets/css/main.css

+}
+
+.rdg-bottom-pinned-rows-sticky-wrapper .react-grid-checkbox-container {


For example only.

We need a generic way to handle this. It is a common requirement to have selection and totals row

qili26 · 2019-10-07T18:05:31Z

examples/scripts/example32-total-rows.js

+    key: 'hours',
+    name: '# of working hours',
+    width: 200,
+    formatter: ({ value }) => <div>{ value.toLocaleString('en-US', { maximumFractionDigits: 0 }) }</div>


Need to display the value with US number format like 1,000

qili26 · 2019-10-07T18:05:56Z

examples/scripts/example32-total-rows.js

+        id: i,
+        task: `Task ${i}`,
+        complete: Math.min(100, Math.round(Math.random() * 110)),
+        priority: ['Critical', 'High', 'Medium', 'Low'][Math.floor(Math.random() * 4)],


Array[0 ~ 4]

qili26 · 2019-10-07T19:07:43Z

packages/react-data-grid/src/ReactDataGrid.tsx

+   * The rows can be used as total rows to display some summary information.
+   * Bottom horizontal scroll bar can move the row left / right. Or a customized row renderer can be used to disabled the scrolling support.
+   */
+  pinnedRows?: R[];


maybe we should use bottomPinnedRows?

The more I think about it, more I incline towards summary/totals row because of a few reasons

pinnedRows can be shown at the bottom or at the top. totals/summaryRows should always be shown at the bottom.

pinnedRows should be treated like regular rows. i.e. it should have a selector, editor and all the features that a regular row has

qili26 · 2019-10-07T19:13:15Z

packages/react-data-grid/src/Canvas.tsx

+  // Set minHeight to show horizontal scrollbar when there are no rows
+  const scrollableRowsWrapperStyle: React.CSSProperties = { width, paddingTop, paddingBottom };
+  const pinnedRowsWrapperStyle: React.CSSProperties = { width };
+  let ieStickyWrapperStyle: React.CSSProperties | undefined;


Any better ideas for IE specific codes?

qili26 · 2019-10-07T19:14:17Z

supersedes #1759

amanmahajan7

Grouping and totaling features are quite interlinked so we can probably keep grouping in mind while designing an API for the totals row. Following is just a rough idea about what we need

groupBy prop. It can be a array of keys [id, name]. For it to work we need to change rowGetter to rows. I am not sure what is the need or rowGetter. Performance may be?
We need a way to show a totals row for the group as well as some way to change group row values. May be we can add some aggregate props to column.
Is there any overlap between group totals and row totals? There may not be any but I want us to start thinking about both features

amanmahajan7 · 2019-10-08T00:44:38Z

packages/react-data-grid/src/Canvas.tsx

-  const paddingTop = rowOverscanStartIdx * rowHeight;
-  const paddingBottom = (rowsCount - 1 - rowOverscanEndIdx) * rowHeight;
+  const pinnedRowsHeight = isIEOrEdge && pinnedRows ? pinnedRows.length * rowHeight : 0;
+  const paddingTop = rowOverscanStartIdx > 0 ? rowOverscanStartIdx * rowHeight : 0;


const paddingTop = rowOverscanStartIdx * rowHeight should be enough

rowOverscanStartIdx will be guaranteed to be a positive integer, right?

Yeah it's 0 minimum.

amanmahajan7 · 2019-10-08T00:56:08Z

examples/assets/css/main.css

+}
+
+.rdg-bottom-pinned-rows-sticky-wrapper .react-grid-checkbox-container {


We need a generic way to handle this. It is a common requirement to have selection and totals row

amanmahajan7 · 2019-10-08T00:58:01Z

packages/react-data-grid/src/Canvas.tsx

@@ -57,6 +58,7 @@ interface RendererProps<R> extends Pick<CanvasProps<R>, 'cellMetaData' | 'onRowS
  isScrolling: boolean;
  colOverscanStartIdx: number;
  colOverscanEndIdx: number;
+  isBottomPinned?: boolean;


I am not 100% sure if we should pass isBottomPinned prop to each formatter. I think we should have a separation between pinnedRows and regular rows. More details in the next comment

amanmahajan7 · 2019-10-08T01:03:40Z

packages/react-data-grid/src/Canvas.tsx

+      return [];
+    }
+
+    return pinnedRows.map((row, idx) => renderRow(row, rowsCount + 1 + idx));


I think pinnedRows should not use renderRow function because of a few reasons

We want to hide checkbox selector on totals row

pinnedRows should not be editable and currently we do not handle it. This results in a weird bug, when you click on the totals cell, selected cell is set to edit mode
https://www.loom.com/share/95ab947e0f91453b99e4f1150296cebf

I am thinking we should have a trimmed down version of renderRow that removes editor and props that are not required. We also do not want all the cell events to fire.

react-data-grid/packages/react-data-grid/src/Cell.tsx

Line 40 in 9d1b648

handleCellClick = () => {

I think adding TotalCell component may be a good idea.

amanmahajan7 · 2019-10-08T01:43:18Z

packages/react-data-grid/src/ReactDataGrid.tsx

+   * The rows can be used as total rows to display some summary information.
+   * Bottom horizontal scroll bar can move the row left / right. Or a customized row renderer can be used to disabled the scrolling support.
+   */
+  pinnedRows?: R[];


The more I think about it, more I incline towards summary/totals row because of a few reasons

pinnedRows can be shown at the bottom or at the top. totals/summaryRows should always be shown at the bottom.

pinnedRows should be treated like regular rows. i.e. it should have a selector, editor and all the features that a regular row has

…data for summary rows / cells

amanmahajan7 · 2019-10-09T12:57:00Z

examples/assets/css/main.css

@@ -0,0 +1,572 @@
+/* Header */
+#head {


I think this file has been deleted

I thought I removed this after merging, will clean this.

amanmahajan7 · 2019-10-09T13:01:09Z

packages/react-data-grid/src/Canvas.tsx

@@ -241,7 +272,8 @@ export default function Canvas<R>({
      colOverscanEndIdx,
      lastFrozenColumnIndex: columnMetrics.lastFrozenColumnIndex,
      isScrolling,
-      scrollLeft
+      scrollLeft,
+      isSummaryRow: summaryRows && summaryRows.includes(row)


This will always be false, correct?

It will always be false only when summaryRows doesn't exist. Same story like isRowSelected, if it's not enabled, it will just be false and populated to next levels.

amanmahajan7 · 2019-10-09T13:04:35Z

packages/react-data-grid/src/Cell.tsx

+                />
+              </span>
+            </div>
+            {tooltip && <span className="cell-tooltip-text">{tooltip}</span>}


I think we should deprecate tooltip prop. Row/Cell renderers must be used to set it and it makes using grid difficult IMO and we should just set the tooltip in the formatter. We do not have to remove tooltip prop in this PR but we can remove it from summaryRow. Our component extensively use this prop so we have to keep in on the regular row for now

I believe we need to update some styles as well. (we should do this in another PR). Currently if we set the tooltip in the formatter, the tooltip will only display when the formatter area is hovered but not the whole cell.

I was referring to that we should remove {tooltip && <span className="cell-tooltip-text">{tooltip}</span>} from this block. We do not need to handle tooltip on the totals row

amanmahajan7 · 2019-10-09T13:07:19Z

packages/react-data-grid/style/rdg-row.less

+  }
+}
+
+.rdg-bottom-pinned-rows {


May be we can use rdg-summary-* classnames to match the prop name

I will change this.

amanmahajan7 · 2019-10-09T13:09:05Z

packages/react-data-grid/src/Cell.tsx

+        >
+          <div className={cellValueClassName}>
+            <div className="react-grid-Cell__container">
+              <span>


We can remove the span?

I want to remove the span, but I am not sure if we should do it in this PR? Maybe in another cell refactor PR?

amanmahajan7 · 2019-10-09T13:10:41Z

packages/react-data-grid/src/Cell.tsx

+          <div className={cellValueClassName}>
+            <div className="react-grid-Cell__container">
+              <span>
+                <CellValue<R>


We can do something like

const cellValue = (<CellValue<R> ... />); if (isSummaryRow) { .... <div className="react-grid-Cell__container"> {cellValue} </div>

Now I think about it, I think it is better to use CellValue for summary rows instead of CellContent. This way we do not have to move the Expand/Collapse functionality here

amanmahajan7 · 2019-10-09T13:26:30Z

packages/react-data-grid/src/Canvas.tsx

@@ -261,6 +293,16 @@ export default function Canvas<R>({
    return <Row<R> {...rendererProps} />;
  }

+  function getRowRef(row: R, idx: number) {


Summary rows ref will never reset from what I can tell. Consider a case

10 rows + 2 summary rows are renderered

rowCount is changed to 30. This will override summary row refs

I am quite sure this will happen but we can double check. We may have to use 2 maps

React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts.

you are right, I will use another way for the summary rows ref.

amanmahajan7 · 2019-10-09T13:27:28Z

examples/demos/example32-summary-rows.js

+
+function DecimalFormatter({ value, isSummaryRow, maximumFractionDigits }) {
+  const text = value.toLocaleString('en-US', maximumFractionDigits == null ? { maximumFractionDigits: 2 } : { maximumFractionDigits: 0 });
+  return isSummaryRow ? <strong>{text}</strong> : <div>{text}</div>;


@MayhemYDG thoughts on the this API where we pass isSummaryRow prop to Row/Cell/Formatters? Another option is to have a dedicated column.GroupFormatter

I'm fine with it.

qili26 · 2019-10-15T02:11:34Z

examples/index.html

@@ -46,6 +46,7 @@
    .example {
      overflow: auto;
      flex-grow: 1;
+      margin: 0 8px;


Added this margin so that I can see the RDG border.

qili26 · 2019-10-15T02:21:32Z

packages/react-data-grid/src/Canvas.tsx

+      </div>
+      {summaryRows && summaryRows.length && (
+        <div className="rdg-summary">
+          <div ref={summary} style={boundaryStyle}>


This is to mimic canvas horizontal scrolling so that for Summary it can just reuse the existing Row.tsx.

The container is fixed width. so that when this div is set the scrollLeft, the rows container (with full width, the next line) could be moving around.

For sticky frozen divs, we do nothing. For IE 11 specifically, we need to set the scrollLeft to each frozen cells. (This logic is just same as what we had)

w3c/csswg-drafts#865
An insteresting finding is that the reason why previously Header sticky doesn't work for EDGE is because we set the .react-grid-HeaderRow as overflow: hidden, it will break the cell position sticky only in EDGE. However, the overflow: hidden like the structure of canvas and summary area would work.

Since EDGE sticky support is buggy, maybe we should consider not using sticky for EDGE? @MayhemYDG

The latest result.

This reverts commit 7fdbfaf.

…Left > 0 bug

🤔

qili26 · 2019-10-17T15:46:38Z

packages/react-data-grid/src/Canvas.tsx

@@ -21,6 +21,7 @@ type SharedDataGridProps<R, K extends keyof R> = Pick<ReactDataGridProps<R, K>,
 | 'RowsContainer'
 | 'getSubRowDetails'
 | 'selectedRows'
+| 'summaryRows'


I cannot add comment to above line so I added here. Just FYI, I found a circular type referencing that we might want to fix in another PR. However, not sure if this is a real problem in terms of types.

https://github.com/adazzle/react-data-grid/pull/1773/files/eee9db5748ec0b44115a398ed8a4d123411fd227..7a99fb7419876457d88de7dcb75a2dbd2abb7cfe#diff-61ebc5f10f9ee6875894862bf0d64cd6R9

https://github.com/adazzle/react-data-grid/blob/canary/packages/react-data-grid/src/RowRenderer.tsx#L6

Circular imports are fine, type imports will be erased in the resulting JS, and proper es module implementations should support circular imports.
https://twitter.com/awbjs/status/1160933193320525825

qili26 · 2019-10-17T15:48:00Z

packages/react-data-grid/src/Canvas.tsx

@@ -78,7 +78,6 @@ export default function Canvas<R, K extends keyof R>({
  const summaryRef = useRef<HTMLDivElement>(null);
  const prevScrollToRowIndex = useRef<number | undefined>();
  const [rowRefs] = useState(() => new Map<number, Row<R>>());
-  const [summaryRowRefs] = useState(() => new Map<number, Row<R>>());


👍 I just realized this is never used after seeing this.

qili26 · 2019-10-17T15:54:44Z

packages/react-data-grid/src/SummaryRowRenderer.tsx

+    <Row<R>
+      idx={idx}
+      row={rowData}
+      width={columnMetrics.totalColumnWidth + getScrollbarSize()}


Header has same calculation, should we add this directly to columnMetrics like columnMetrics.fullWidth? (Looks like we had this before but removed)

Maybe we'll do that when we'll refactor header/summary rows to render them directly in the grid (if we can work around the edge bug) 🤔

feat: manually apply total rows implementation codes based on canary …

f02154f

…branch

qili26 self-assigned this Oct 7, 2019

qili26 commented Oct 7, 2019

View reviewed changes

qili26 added 2 commits October 7, 2019 13:08

chores: minor tweak

04b7e0b

chores: fix idx

4470337

qili26 commented Oct 7, 2019

View reviewed changes

chores: example small update

9d1b648

qili26 commented Oct 7, 2019

View reviewed changes

qili26 requested review from nstepien and amanmahajan7 October 7, 2019 19:14

amanmahajan7 reviewed Oct 8, 2019

View reviewed changes

qili26 added 5 commits October 8, 2019 15:41

chores: rename summaryRows; avoidhandling events or some unnecessary …

71b37a6

…data for summary rows / cells

chores: catchup canary

506ec57

fix: use correct interaction mask reference

6534d41

fix: merge issues

3eba2a4

feat: hide summary row checkbox

cba04c5

amanmahajan7 previously requested changes Oct 9, 2019

View reviewed changes

qili26 added 3 commits October 9, 2019 12:44

Merge branch 'canary' into ql-total-rows

e14412e

chores: remove undeleted files due to merging

15344f8

chores: refactor some codes

223c7f3

qili26 changed the title ~~Bottom pinned rows~~ Summary rows Oct 9, 2019

qili26 added 4 commits October 9, 2019 15:06

chores: refactor canvas

2f63e2d

chores: refactor Cell.tsx

0f8f19c

chores: remove unused CellContent

f7a5d73

fix: type reference

6e3e518

qili26 marked this pull request as ready for review October 9, 2019 20:26

qili26 added the Needs Tests label Oct 9, 2019

qili26 added 3 commits October 14, 2019 16:32

chores: css / scrolling logic tweak

2f8cb85

chores: tweak style

4798d13

fix: broken scrolling logic

1969318

qili26 commented Oct 15, 2019

View reviewed changes

qili26 added 12 commits October 14, 2019 21:24

chores: remove dead code

ea579a9

chores: catchup dev

5ae4082

chores: remove cellContent

7fdbfaf

Revert "chores: remove cellContent"

f35903d

This reverts commit 7fdbfaf.

chores: add cellContent back

542d9c4

fix: broken behavior by merging

d38bd4e

chores: catchup canary

a512fe4

chores: add SummaryRowRenderer

6077d0b

chores: organize the code a little bit

f534772

chores: catchup canary

e9abe1e

fix: type issue due to merge

4884298

chores: sync scroll

34df275

qili26 requested a review from amanmahajan7 October 16, 2019 17:36

qili26 and others added 7 commits October 16, 2019 15:38

chores: catchup canary

1495fef

fix: type and summary row scrollLeft and height

00a4d78

chores: revert scroll sync logic due to IE overflow-x:hidden / scroll…

fab0ada

…Left > 0 bug

chores: move boundary style in if

c011068

Merge branch 'canary' into ql-total-rows

eee9db5

Clean up scrolling

7a99fb7

Fix canvas height calculation, nits

2a1b919

nstepien approved these changes Oct 17, 2019

View reviewed changes

qili26 commented Oct 17, 2019

View reviewed changes

nstepien merged commit 0f40988 into canary Oct 17, 2019

nstepien deleted the ql-total-rows branch October 17, 2019 16:00

		}

		.rdg-bottom-pinned-rows-sticky-wrapper .react-grid-checkbox-container {

Summary rows #1773

Summary rows #1773

Conversation

qili26 commented Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qili26 Oct 7, 2019 • edited Loading

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

qili26 commented Oct 7, 2019

amanmahajan7 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

Choose a reason for hiding this comment

amanmahajan7 Oct 9, 2019 • edited by qili26 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanmahajan7 Oct 9, 2019 • edited Loading

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

qili26 Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

qili26 Oct 15, 2019 • edited Loading

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

qili26 commented Oct 7, 2019 •

edited

Loading

qili26 Oct 7, 2019 •

edited

Loading

amanmahajan7 Oct 9, 2019 •

edited by qili26

Loading

amanmahajan7 Oct 9, 2019 •

edited

Loading

qili26 Oct 15, 2019 •

edited

Loading

qili26 Oct 15, 2019 •

edited

Loading