#870 - Add PivotTableRenderer to bold #876

wagnerbsantos · 2024-11-13T00:15:12Z

closes #870
The component receives data in a Tree data structure and outputs a table using the desired keys as headers.

wagnerbsantos · 2024-11-13T00:21:07Z

src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

These functions transform the table calculated with getResult into a list of PivotTableCell. I didn't find a way to test this code directly.

Thats why you usually don't return React Components from util functions 😬 This should be either (a) a component that renders the cells or (b) a hook usePivotTableCells that returns an array of PivotTableCellProp that you can use in the component along with a .map to render the Cells. Here you are mixing util functions with components an this makes this behavior too complex AND makes it really hard to implement tests. Dangerous combination.

Maybe a combination of (a) and (b) is good too: Make a HorizontalPivotTable, VerticalPivotTable, etc that uses a hook/util function to calculate its props sounds good.

I chose to follow the (b) approach. It will be simpler to test.

zDudaHang

Nice!
I'm requesting changes because i think the export button is missing

.loki/reference/chrome_laptop_Components_PivotTable_PivotTableRenderer_All_Types.png

src/components/PivotTable/Droppable/Droppable.tsx

src/components/PivotTable/PivotTableCell/PivotTableCell.stories.tsx

src/components/PivotTable/PivotTableCell/PivotTableProvider.tsx

src/components/PivotTable/PivotTableRenderer/util.ts

src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

zDudaHang · 2024-11-13T13:54:42Z

src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

+  return { divs, rowTotalValues, totalRowNumber: maxRowEnd, cellPosition }
+}
+
+function getVertical<T extends object>({


The same comments I made in previous method can be applied here

zDudaHang · 2024-11-13T13:55:13Z

.loki/reference/chrome_laptop_Components_PivotTable_PivotTableRenderer_All_Types.png

I think the Export Button is missing here

Exporting the table will be done in another issue.

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.tsx

src/components/PivotTable/PivotTableRenderer/model-pivotTableRenderer.ts

wesleydecezere

🚀

src/components/PivotTable/PivotTableRenderer/model-pivotTableRenderer.ts

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.tsx

src/components/PivotTable/PivotTableRenderer/util.ts

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.tsx

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.test.tsx

src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

wesleydecezere · 2024-11-18T12:18:44Z

src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

+  /**
+   * Create vertical header
+   */
+  keys.forEach((k, idx) => {
+    const headerGridArea = new GridArea(headerSpace, idx + 1, headerSpace + 1, idx + 2)
+    divs.push(
+      <PivotTableCell
+        types={new Set([PivotTableCellType.HEADER])}
+        key={headerGridArea.toString()}
+        gridArea={headerGridArea}
+      >
+        {keysMapping.get(k).keyName}
+      </PivotTableCell>
+    )
+  })


src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.stories.tsx

wesleydecezere

Nice!

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.tsx

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.test.tsx

src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

zDudaHang · 2024-11-21T12:37:27Z

src/components/PivotTable/PivotTableRenderer/PivotTableRenderer.test.tsx

+  }
+  const maxLeafValue = props.defaultTree.maxLeafValue as number
+  return (
+    <PivotTableProvider maxValue={maxLeafValue} suffix={''}>


Suggested change

<PivotTableProvider maxValue={maxLeafValue} suffix={''}>

<PivotTableProvider maxValue={maxLeafValue} suffix=''>

Andrevmatias

Wow! It's a really complex component! Great!

I think you can improve the readability and maintainability by splitting some complex code and giving better names for the vars and functions.

src/components/PivotTable/PivotTableRenderer/styles.ts

src/components/PivotTable/PivotTableRenderer/PivotTableBuilder.tsx

src/components/PivotTable/PivotTableRenderer/styles.ts

src/components/PivotTable/PivotTableRenderer/classes/GroupResult.ts

src/components/PivotTable/PivotTableRenderer/model.ts

src/components/PivotTable/PivotTableRenderer/util.ts

src/components/PivotTable/PivotTableRenderer/utils.test.ts

Andrevmatias

When working inside a lib take extra care about leaving commented code and "console.log". The lib code is already big enough and console.log can compromise the performance and debug of the systems that use this lib.

Most of the other comments are only suggestions.

src/components/PivotTable/PivotTableGrid/utils.test.ts

src/components/PivotTable/PivotTableGrid/PivotTableGrid.tsx

src/components/PivotTable/PivotTableGrid/model.ts

src/components/PivotTable/PivotTableGrid/util.ts

src/components/PivotTable/PivotTableGrid/model.ts

Andrevmatias

Beautiful! 😍

wagnerbsantos added 3 commits November 12, 2024 17:16

util test done

48d0758

Finish tests

f54e1d6

remove comments

8cfcfa5

wagnerbsantos commented Nov 13, 2024

View reviewed changes

Fix comments

d6cbfa8

wagnerbsantos requested review from wesleydecezere, AndresKindel, zDudaHang and GuilhermePrandi November 13, 2024 00:32

zDudaHang requested changes Nov 13, 2024

View reviewed changes

simple review fixes

54ff3bd

wesleydecezere previously approved these changes Nov 18, 2024

View reviewed changes

wesleydecezere removed the request for review from GuilhermePrandi November 18, 2024 14:17

Almost all review fixes

964dfb4

wagnerbsantos dismissed wesleydecezere’s stale review via 964dfb4 November 18, 2024 17:29

separate functions

a96d962

wagnerbsantos requested review from zDudaHang and wesleydecezere November 18, 2024 19:35

wesleydecezere previously approved these changes Nov 19, 2024

View reviewed changes

Test changed names and broke

60570dd

wagnerbsantos dismissed wesleydecezere’s stale review via 60570dd November 19, 2024 16:20

wagnerbsantos requested a review from Andrevmatias November 19, 2024 16:24

zDudaHang previously approved these changes Nov 21, 2024

View reviewed changes

Andrevmatias self-assigned this Nov 21, 2024

fix review

14b3e3b

wagnerbsantos dismissed zDudaHang’s stale review via 14b3e3b November 22, 2024 13:49

Andrevmatias requested changes Nov 27, 2024

View reviewed changes

Andrevmatias removed their assignment Nov 27, 2024

wagnerbsantos added 2 commits December 3, 2024 13:42

Big refactor safety commit

01f666d

Refactoring done

4134bae

wagnerbsantos requested a review from Andrevmatias December 5, 2024 17:52

Andrevmatias self-assigned this Dec 6, 2024

Andrevmatias requested changes Dec 9, 2024

View reviewed changes

Andrevmatias removed their assignment Dec 9, 2024

fabmatos and others added 3 commits December 12, 2024 08:23

Merge branch 'main' into 870

467ac64

Fix review

2f41036

Merge branch '870' of github.com:laboratoriobridge/bold into 870

e225f47

wagnerbsantos requested a review from Andrevmatias December 19, 2024 13:29

Andrevmatias self-assigned this Dec 20, 2024

Andrevmatias approved these changes Dec 20, 2024

View reviewed changes

Andrevmatias removed their assignment Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#870 - Add PivotTableRenderer to bold #876

#870 - Add PivotTableRenderer to bold #876

wagnerbsantos commented Nov 13, 2024 •

edited

Loading

wagnerbsantos Nov 13, 2024

Andrevmatias Nov 27, 2024

wagnerbsantos Dec 3, 2024

zDudaHang left a comment

zDudaHang Nov 13, 2024

zDudaHang Nov 13, 2024

wagnerbsantos Nov 14, 2024

wesleydecezere left a comment

wesleydecezere Nov 18, 2024

wesleydecezere left a comment

zDudaHang Nov 21, 2024

Andrevmatias left a comment

Andrevmatias left a comment

Andrevmatias left a comment

	<PivotTableProvider maxValue={maxLeafValue} suffix={''}>
	<PivotTableProvider maxValue={maxLeafValue} suffix=''>

#870 - Add PivotTableRenderer to bold #876

Are you sure you want to change the base?

#870 - Add PivotTableRenderer to bold #876

Conversation

wagnerbsantos commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zDudaHang 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

wesleydecezere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleydecezere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andrevmatias left a comment

Choose a reason for hiding this comment

Andrevmatias left a comment

Choose a reason for hiding this comment

Andrevmatias left a comment

Choose a reason for hiding this comment

wagnerbsantos commented Nov 13, 2024 •

edited

Loading