WIP: React version of visualizations #2988

washort · 2018-10-19T04:23:23Z

Needs another round of testing for all visualizations but this should cover all the basics.

This is adapted from my larger branch containing a React implementation of the entire /queries/ page.

TODO:

Pull Requests to sync:

Unresolved issues:

Table: grid size default is 250, but needs to be 25.
Filters don't render on dashboards (see).
Counter visualization: when picking a non number value it will: 1. show cryptic React error; 2. stop working even if you select a valid input.
Sankey doesn't resize on available space change (see).
NavBar is broken due to misplaced Ant styles on Chrome.
Counter target values don't render (https://deploy-preview-2988--redash-preview.netlify.com/queries/32#54 vs. https://redash-preview.netlify.com/queries/32#54).

arikfr

Thanks! This is a huge milestone. Both in scope and the progress it brings to our move to React.

We will need to figure out a way to how to do it in a way that minimizes the (negative) impact on the application's stability/backward support. One idea I had is to create a few test case visualizations for each visualization type that we can render with the current implementation and this new one to compare for changes and issues.

Also, we need to keep track of visualizations related pull requests we merge while this is in review/development.

The included comments are from a quick review, a more detailed one will follow.

package.json

arikfr · 2018-10-21T06:21:14Z

package.json

@@ -4,10 +4,10 @@
  "description": "The frontend part of Redash.",
  "main": "index.js",
  "scripts": {
-    "start": "webpack-dev-server",
+    "start": "node --max-old-space-size=4096 node_modules/.bin/webpack-dev-server",


😮 build was failing without this? somewhat disturbing.

I backed this out and CircleCI started failing with heap space exhausted error again, so I think so. Not sure if there's something we can do to get webpack to run leaner.

package.json

client/app/visualizations/registry.js

client/app/visualizations/map/MapEditor.jsx

arikfr · 2018-10-21T08:43:53Z

@jezdez @kravets-levko @washort Let's keep a list of changes done to visualizations in other pull requests, so we know what to sync.

Table visualization: change default size to 25 and add more size options #2982 Table visualization: change default size to 25 and add more size options
Add: Heatmap chart visualization by Plotly #2080 Plotly: add heatmaps support
Plotly's toolbox covers part of the chart #3023
Charts lose responsive features after refreshing the dashboard #3024 (not sure if needed)

arikfr · 2018-10-28T13:34:42Z

Can you please rebase with master, so we can start testing this with the new Netlify previews (#2999)? You can even just copy the Netlify config file introduced in #2999 if rebasing generates too much conflicts/work.

Thanks!

washort · 2018-10-29T16:19:01Z

Rebased on master, all review comments addressed. I believe this is caught up now -- I think the react stuff handles relayout so #3024 probably doesn't need any attention.

arikfr · 2018-10-29T20:04:19Z

Some issues I found from testing this for a few minutes:

The navbar is broken because of Ant's styles -- you need to move the Select component styles to the ant.less file.
The table doesn't render for unsaved queries.
Table visualization: sorting no longer works.
Table visualization editor: in any input field, after each change (add/remove character), it loses focus.
Table visualization editor: the grid size option seems to have no effect.
Chart visualization editor: the forms look broken, but maybe this is because of Ant's style collisions?

washort · 2018-10-30T23:39:51Z

Fixed, except for the styles in the chart editor -- I'm not a CSS expert so it's not obvious to me what's going on there.

arikfr · 2018-11-07T15:41:40Z

I did another run at it on the preview instance, and here's a few things I stumbled at:

When enabling search in a table visualization, it doesn't actually filter. Example
Table pagination doesn't work. Example
Table: grid size default is 250, but needs to be 25.
Filters: they don't look the same as before (title next to drop down). Example
Multi Filters: don't work (show some React error). Example
NavBar is broken due to misplaced Ant styles.
Charts: legend click event don't work. Example
Charts: we need to hide the Plotly logo in the toolbox.

We might want to find a more scalable to document these issues. Maybe Google Sheets? Or just update a single comment over here?

washort · 2018-11-08T22:14:10Z

Fixed the above; however I wasn't able to reproduce a couple -- 25 seems to be the default page size for new tables. Also, what's the issue with the navbar? this is what I see:

arikfr · 2018-11-09T08:03:51Z

25 seems to be the default page size for new tables.

I'll check that again.

Also, what's the issue with the navbar? this is what I see:

Apparently it's a Chrome only issue 😮Anyway, it's an Ant issue. @kravets-levko @kocsmy which one of you fixed it last? Can take a look? :)

arikfr · 2018-11-12T08:13:30Z

Table visualization horizontal scrollbar should not be always visible #3061

arikfr · 2018-11-12T08:17:45Z

I started tracking open issues in the pull request description to make it easier to find and see what's still open.

arikfr · 2019-01-17T14:21:05Z

Thank you, @washort, for the continued effort here. Unfortunately this been open for a long time and there seems to be no end on the horizon. Visualizations play a critical role in Redash, so we need high confidence that we didn't break anything before merging this.

There are still some small issues with the visualizations and all the visualizations options editors still require an update.

Considering all of the above, @ranbena will take over the effort of migrating visualizations to React. He will use the code from this pull request, but introduce each visualizations in a separate pull request. So we can gradually migrate. I know we discussed this option in the past, and decided to keep going with the monolith pull request, but: at the time I didn't expect this PR to be open for another 2 months and I wanted to save you the trouble of splitting this... :)

We will keep this open for now, but probably close once the first split PR lands.

We will track the migration work using #3301.

brylie · 2019-05-27T08:48:16Z

client/app/visualizations/chart/ChartEditor.jsx

+                updateOptions={pie ? this.updateValuesOptions : this.updateSeriesOptions}
+              />
+            </Tabs.TabPane> : null }
+          {opts.globalSeriesType !== 'custom' ?


These ternary operators probably shouldn't be spread across so many lines, as it makes the code difficult to read and understand.

I think this is partially a side-effect of JSX allowing full imperative logic intermixed with declarative markup. It somewhat presents a footgun, whereby developers are writing obscure and overly clever code, where a simpler template syntax would suffice.

brylie · 2019-05-27T08:53:38Z

client/app/visualizations/chart/ChartEditor.jsx

+          {opts.globalSeriesType !== 'custom' ?
+            <Tabs.TabPane key="dataLabels" tab="Data Labels">
+              <div className="m-t-10 m-b-10">
+                {includes(['line', 'area', 'column', 'scatter', 'pie'], opts.globalSeriesType) ?


Is there an alternative approach than multi-line ternary operators?

brylie · 2019-05-27T08:55:29Z

client/app/visualizations/chart/ChartRenderer.jsx

+      const definitionParts = definition.split('::') || definition.split('__');
+      const name = definitionParts[0];
+      const type = mapping ? mapping[definition] : definitionParts[1];
+      let value = v;


Can you rename v to value in the iterator, or are arguments treated as const?

Hi @brylie! This PR is going to be abandoned - we already implemented a part of it in #3493 and will migrate other visualizations one by one for better testability (@washort did a really good job here, but he decided to migrate all at once, and it was too hard to test everything and fix bugs. we keep this PR as an example for a future smaller PRs).

arikfr · 2019-05-27T11:16:17Z

I'll close this now, to avoid further confusion. I guess we can use it as reference without it lingering in the PRs list :-)

Thanks, @washort for showing the way :)

washort force-pushed the react-visualizations branch from b52a316 to 29ddbc6 Compare October 19, 2018 04:26

washort added Frontend Visualizations labels Oct 19, 2018

washort mentioned this pull request Oct 19, 2018

React component for plotly charts #2633

Closed

washort force-pushed the react-visualizations branch 2 times, most recently from 1313118 to 5fd2cb6 Compare October 19, 2018 17:24

arikfr reviewed Oct 21, 2018

View reviewed changes

washort force-pushed the react-visualizations branch 4 times, most recently from f11c02d to 6ec0332 Compare October 27, 2018 02:45

washort force-pushed the react-visualizations branch from 6ec0332 to 6619df9 Compare October 29, 2018 15:01

washort force-pushed the react-visualizations branch from 6619df9 to f7541d2 Compare October 30, 2018 23:38

washort force-pushed the react-visualizations branch 2 times, most recently from 0fb09fb to 70f0970 Compare November 6, 2018 20:05

washort force-pushed the react-visualizations branch 2 times, most recently from 5d032c3 to 495a3a9 Compare November 8, 2018 21:56

arikfr mentioned this pull request Nov 9, 2018

ECharts supports #2974

Closed

washort force-pushed the react-visualizations branch from 495a3a9 to 052a489 Compare November 9, 2018 17:14

arikfr changed the title ~~React version of visualizations~~ WIP: React version of visualizations Nov 12, 2018

Allen Short added 20 commits January 14, 2019 14:34

update react

7f9060f

Resize Sankey visualizations, show more informative error messages

8ce0e32

fix filters

b0ddc34

fix up counter editor

aecb48e

catch up with getredash#3102

62452df

general resize support

9438e2a

render counters properly

1b19b75

apply z-index fix to ant select

f62d5bf

Better fix for resizing

076069f

fixes for multiple Y columns

5d975a9

choropleth fixes

11be560

use autosizer to scale counter

1884f15

display vis name in dashboard

d4702ba

catch up with getredash#3206

90555d7

handle resize/move of counter vis

ca4653e

match original bounds/zoom behavior

c41ffbf

proper display in editor

50f70a3

update choropleth vis in editor

31e733d

enable pace

46fc934

ant-compatible footer

4626ee4

washort force-pushed the react-visualizations branch from a667488 to 4626ee4 Compare January 14, 2019 20:34

emtwo mentioned this pull request Jan 31, 2019

Coerce to moment when 'datetime' selected by user. #3150

Merged

kravets-levko mentioned this pull request Mar 5, 2019

Migrate visualizations registry/renderer/editor to React #3493

Merged

25 tasks

brylie reviewed May 27, 2019

View reviewed changes

arikfr closed this May 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: React version of visualizations #2988

WIP: React version of visualizations #2988

washort commented Oct 19, 2018 •

edited

Loading

arikfr left a comment

arikfr Oct 21, 2018

washort Oct 26, 2018

arikfr commented Oct 21, 2018 •

edited by washort

Loading

arikfr commented Oct 28, 2018 •

edited

Loading

washort commented Oct 29, 2018

arikfr commented Oct 29, 2018

washort commented Oct 30, 2018

arikfr commented Nov 7, 2018 •

edited by washort

Loading

washort commented Nov 8, 2018

arikfr commented Nov 9, 2018

arikfr commented Nov 12, 2018 •

edited

Loading

arikfr commented Nov 12, 2018

arikfr commented Jan 17, 2019

brylie May 27, 2019

brylie May 27, 2019

brylie May 27, 2019

kravets-levko May 27, 2019

arikfr commented May 27, 2019

WIP: React version of visualizations #2988

WIP: React version of visualizations #2988

Conversation

washort commented Oct 19, 2018 • edited Loading

arikfr left a comment

Choose a reason for hiding this comment

arikfr Oct 21, 2018

Choose a reason for hiding this comment

washort Oct 26, 2018

Choose a reason for hiding this comment

arikfr commented Oct 21, 2018 • edited by washort Loading

arikfr commented Oct 28, 2018 • edited Loading

washort commented Oct 29, 2018

arikfr commented Oct 29, 2018

washort commented Oct 30, 2018

arikfr commented Nov 7, 2018 • edited by washort Loading

washort commented Nov 8, 2018

arikfr commented Nov 9, 2018

arikfr commented Nov 12, 2018 • edited Loading

arikfr commented Nov 12, 2018

arikfr commented Jan 17, 2019

brylie May 27, 2019

Choose a reason for hiding this comment

brylie May 27, 2019

Choose a reason for hiding this comment

brylie May 27, 2019

Choose a reason for hiding this comment

kravets-levko May 27, 2019

Choose a reason for hiding this comment

arikfr commented May 27, 2019

washort commented Oct 19, 2018 •

edited

Loading

arikfr commented Oct 21, 2018 •

edited by washort

Loading

arikfr commented Oct 28, 2018 •

edited

Loading

arikfr commented Nov 7, 2018 •

edited by washort

Loading

arikfr commented Nov 12, 2018 •

edited

Loading