Migrate Pivot Table visualization to React #4133

gabrieldutra · 2019-09-11T00:00:37Z

What type of PR is this? (check all applicable)

Refactor

Description

Migration for the Pivot Table Visualization

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

kravets-levko · 2019-09-11T07:15:50Z

client/app/components/Filters.jsx

@@ -1,10 +1,10 @@
-import { isArray, indexOf, get, map, includes, every, some, toNumber, toLower } from 'lodash';
+import { isArray, indexOf, get, map, includes, every, some, toNumber } from 'lodash';


Is this related to this PR?

Yes, I faced an issue with the react version, it doesn't handle Moment values as the jQuery version does. The jQuery tries to stringfy it, while the react version just breaks the whole thing.

So I was thinking about moving the logic we have to normalize row values for Filters somewhere shared and use it here as well.

gabrieldutra · 2019-09-12T23:52:39Z

The React version doesn't have the rowTotals and colTotals options (it seems someone implemented that in plotly/react-pivottable#79, however it also seems inactive 😅), so I went with a css approach to that.

gabrieldutra · 2019-09-12T23:55:23Z

client/app/visualizations/pivot/renderer.less

+
+  &[data-hide-row-totals] {
+    td:last-child, th:last-child {
+      &.pvtTotalLabel:not(:empty), &.pvtTotal, &.pvtGrandTotal {


the :not(:empty) here avoids us to hide this cell (that I actually have no idea why it now has .pvtTotalLabel):

client/app/visualizations/pivot/index.js

- cy.all with multiple args - add controls to pivot valid options

gabrieldutra

@kravets-levko considering this was my first work related to React Visualizations, lmk if I forgot ath 🙂.

gabrieldutra · 2019-09-16T21:15:15Z

client/app/components/Filters.jsx

                    allowClear={filter.multiple}
-                    filterOption={(searchText, option) => includes(toLower(option.props.children), toLower(searchText))}
+                    optionFilterProp="children"


Took the opportunity to simplify this

gabrieldutra · 2019-09-16T21:16:09Z

client/app/filters/index.js

@@ -168,3 +169,18 @@ export function join(arr) {

  return arr.join(' / ');
 }
+
+export function formatColumnValue(value, columnType = null) {


Just moved this from Filters.jsx. This seemed the best place to put it, LMK if you have somewhere else in mind.

gabrieldutra · 2019-09-16T21:25:27Z

client/app/visualizations/pivot/renderer.less

@@ -13,16 +29,6 @@
  background: #fff;
 }

-.pvtUi {
-  td, th {
-    padding: 5px;


5px is the default now

gabrieldutra · 2019-09-16T21:26:14Z

client/app/visualizations/pivot/renderer.less

-    padding: 5px;
-  }
-
-  li.ui-sortable-handle {


I didn't find any .ui-sortable-handle, also it didn't seem that important

.ui-<whatever> prefixes are related to jQuery.UI - I hope React components don't use it 😁

gabrieldutra · 2019-09-16T21:30:11Z

client/cypress/integration/visualizations/pivot_spec.js

+          cy.getByTestId(getWidgetTestId(widget))
+            .within(() => cy.getByTestId('PivotTableVisualization').should('exist'));
+        });
+        cy.percySnapshot('Visualizations - Pivot Table');


It seemed too hard to make sure those features won't break by assertions, so I went with Percy for the job and created a Dashboard with different Pivot Tables 😁. Also used the API for everything.

Added a cy.all to work as a Promise.all for Cypress chains (related issue).

kravets-levko

Looks really good 👍 I still want to test it on preview instance, and left a few comments (not a blocker though - everything can be kept as is)

kravets-levko · 2019-09-17T09:54:17Z

client/app/visualizations/pivot/Renderer.jsx

+
+  useEffect(() => {
+    setConfig({ data: formatRows(data), ...options });
+  }, [data, options]);


I think it could be replaced with useMemo; also, you can remove setConfig from onChange: updated options will do a trip through onOptionsChange and chart editor and return back to the component where useMemo will pick them up (I'm not 100% sure if it will work, but I hope so 😁):

const config = useMemo( () => ({ data: formatRows(data), ...options }), [data, options] );

My reasoning for using a state here is that from my understanding onOptionsChange will only "exist" (something else than () => {}) when in Editor context. However, for the Pivot Table it has the possibility of keeping the controls in the visualization, so I wanted to keep them updating internally even though it's not persisted.

In a more practical way, this is what happens when I use useMemo:

Got it 👍 Then let's keep as is

kravets-levko · 2019-09-17T09:56:02Z

client/app/visualizations/pivot/renderer.less

-    padding: 5px;
-  }
-
-  li.ui-sortable-handle {


.ui-<whatever> prefixes are related to jQuery.UI - I hope React components don't use it 😁

arikfr · 2019-09-22T07:40:24Z

From the screenshots it looks like the Pivot table has a different font than the rest of the application. Is it like this for the regular pivot table for you as well?

arikfr · 2019-09-22T07:46:11Z

👍

gabrieldutra added 2 commits September 10, 2019 20:53

npm install react-pivottable

324ea07

Initiate Pivot Table Migration

489c8d9

gabrieldutra added Frontend Visualizations Frontend: React Frontend codebase migration to React labels Sep 11, 2019

gabrieldutra self-assigned this Sep 11, 2019

arikfr mentioned this pull request Sep 11, 2019

Migrate Visualizations to React #3301

Closed

24 tasks

kravets-levko reviewed Sep 11, 2019

View reviewed changes

gabrieldutra added 2 commits September 12, 2019 18:43

Merge branch 'master' into react-pivottable

09cf7a4

Update renderer with editor options

bace703

gabrieldutra commented Sep 12, 2019

View reviewed changes

kravets-levko reviewed Sep 13, 2019

View reviewed changes

client/app/visualizations/pivot/index.js Outdated Show resolved Hide resolved

gabrieldutra added 8 commits September 13, 2019 19:15

Clean up

c82a4c6

Remove old pivottable from package.json

4032c1c

Test Percy Snapshot with Pivot Table in a Dashboard

9a3c74b

Merge branch 'master' into react-pivottable

dd43500

Tmp: use cy.wait to make sure dashboard is loaded

26090dd

Clean up Percy snapshot test

b7b325e

Small improvements

88096e2

- cy.all with multiple args - add controls to pivot valid options

Watch for options in the Renderer

d8a9a89

gabrieldutra commented Sep 16, 2019

View reviewed changes

gabrieldutra requested a review from kravets-levko September 16, 2019 22:15

gabrieldutra marked this pull request as ready for review September 16, 2019 22:15

weekly-digest bot mentioned this pull request Sep 16, 2019

Weekly Digest (9 September, 2019 - 16 September, 2019) #4148

Closed

kravets-levko reviewed Sep 17, 2019

View reviewed changes

gabrieldutra added 2 commits September 18, 2019 18:36

Merge branch 'master' into react-pivottable

c5bb351

Merge branch 'master' into react-pivottable

842e7a3

arikfr merged commit fd435d2 into master Sep 22, 2019

arikfr deleted the react-pivottable branch September 22, 2019 07:46

weekly-digest bot mentioned this pull request Sep 23, 2019

Weekly Digest (16 September, 2019 - 23 September, 2019) #4172

Closed

MaxMood96 mentioned this pull request Nov 28, 2023

[Snyk] Fix for 16 vulnerabilities MaxMood96/redash#129

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Pivot Table visualization to React #4133

Migrate Pivot Table visualization to React #4133

gabrieldutra commented Sep 11, 2019 •

edited

Loading

kravets-levko Sep 11, 2019

gabrieldutra Sep 11, 2019

gabrieldutra commented Sep 12, 2019

gabrieldutra Sep 12, 2019

gabrieldutra left a comment

gabrieldutra Sep 16, 2019

gabrieldutra Sep 16, 2019

gabrieldutra Sep 16, 2019

gabrieldutra Sep 16, 2019

kravets-levko Sep 17, 2019

gabrieldutra Sep 16, 2019

kravets-levko left a comment

kravets-levko Sep 17, 2019

gabrieldutra Sep 17, 2019

kravets-levko Sep 17, 2019

kravets-levko Sep 17, 2019

arikfr commented Sep 22, 2019

arikfr commented Sep 22, 2019

		@@ -1,10 +1,10 @@
		import { isArray, indexOf, get, map, includes, every, some, toNumber, toLower } from 'lodash';
		import { isArray, indexOf, get, map, includes, every, some, toNumber } from 'lodash';

Migrate Pivot Table visualization to React #4133

Migrate Pivot Table visualization to React #4133

Conversation

gabrieldutra commented Sep 11, 2019 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Sep 12, 2019

Choose a reason for hiding this comment

gabrieldutra 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

kravets-levko 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

arikfr commented Sep 22, 2019

arikfr commented Sep 22, 2019

gabrieldutra commented Sep 11, 2019 •

edited

Loading