Migrate Chart visualization to React Part 1: Renderer #4130

kravets-levko · 2019-09-09T18:40:00Z

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

Refactor

Description

Convert component to React
- "normal" chart
- custom chart
Cleanup

Related Tickets & Documents

#3301 (Migrate Visualizations to React -> Chart)

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

No visual changes.

arikfr · 2019-09-09T18:50:16Z

We won't gain anything from using their React wrapper?

…

On Mon, Sep 9, 2019, 21:40 Levko Kravets ***@***.***> wrote: What type of PR is this? (check all applicable) - Refactor Description - Convert component to React - "normal" chart - custom chart - Cleanup - More tests (?) Related Tickets & Documents #3301 <#3301> (Migrate Visualizations to React -> Chart) Mobile & Desktop Screenshots/Recordings (if there are UI changes) Should be no changes, but will see soon ------------------------------ You can view, comment on, or merge this pull request online at: #4130 Commit Summary - Migrate Chart visualization: Renderer File Changes - *A* client/app/visualizations/chart/Renderer/CustomPlotlyChart.jsx <https://github.com/getredash/redash/pull/4130/files#diff-0> (8) - *A* client/app/visualizations/chart/Renderer/PlotlyChart.jsx <https://github.com/getredash/redash/pull/4130/files#diff-1> (69) - *A* client/app/visualizations/chart/Renderer/index.jsx <https://github.com/getredash/redash/pull/4130/files#diff-2> (14) - *M* client/app/visualizations/chart/index.js <https://github.com/getredash/redash/pull/4130/files#diff-3> (4) - *M* client/app/visualizations/chart/plotly/index.js <https://github.com/getredash/redash/pull/4130/files#diff-4> (8) Patch Links: - https://github.com/getredash/redash/pull/4130.patch - https://github.com/getredash/redash/pull/4130.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4130?email_source=notifications&email_token=AAAROLC5M77CYO2DYAUJXLDQI2KAFA5CNFSM4IU6W6PKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HKIA56Q>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAROLDJ5SIAWRWPR2GUCFDQI2KAFANCNFSM4IU6W6PA> .

kravets-levko · 2019-09-09T19:04:11Z

@arikfr Not really - we don't use almost anything from the features it offers

kravets-levko · 2019-09-10T10:48:11Z

client/app/visualizations/chart/Renderer/PlotlyChart.jsx

+      // It will auto-purge previous graph
+      Plotly.newPlot(container, plotlyData, plotlyLayout, plotlyOptions).then(() => {
+        applyLayoutFixes(container, plotlyLayout, (e, u) => Plotly.relayout(e, u));
+      });


@arikfr That's actually all we need from Plotly here + few Plotly.relayout calls below. That's why I think we don't need extra dependency of react-plotly.js - it will not allow us to get rid of plotly.jsanyway because React component is just a wrapper around it.

ranbena

LGTM. Functions well. One question in code.

ranbena · 2019-09-11T06:42:52Z

client/app/visualizations/chart/Renderer/PlotlyChart.jsx

+        applyLayoutFixes(container, plotlyLayout, (e, u) => Plotly.relayout(e, u));
+      });
+
+      container.on('plotly_restyle', (updates) => {


Shouldn't this be moved to the below useEffect()?

I'm not sure I understand your point. It uses some variables from the same scope, so cannot be moved to another useEffect without moving that variables to upper scope. If you mean that both useEffect calls should be merged - probably yes, but current variant is a sort of optimization: first useEffect will re-build chart when container, options and/or data changes - Plotly.newPlot will purge old chart if needed. And we need to call Plotly.purge only when container changes.

It uses some variables from the same scope

Perhaps one of us is misunderstanding useEffect's 2nd argument usage.
AFAIK, it's used as "componentshouldupdate". You can use any variables in the useEffect scope without adding them to the 2nd argument. Am I wrong about this?

My concern here is that container.on is being re-declared needlessly every time options and data change, but perhaps needs to only when container does.

container.on should be called after each Plotly.newPlot because Plotly does a sort of shitty thing: it extends DOM element which is a plot container and adds new properties and methods to it, like on. So when you call Plotly.newPlot - it will destroy previous plot and unbind all events, and then you have to re-bind everything again. If that was a regular DOM events (addEventListener) - then yes, I totally agree with you

arikfr · 2019-09-12T07:23:51Z

👍

* Migrate Parameters component to React (#4006) * Start Parameters Migration * Add dirtyCount * Use workaround with setState * Apply Changes * Add EditSettingsDialog * Add Cmd/Ctrl + Enter behavior * Remove isApplying * Delete Angular version of parameters * Update tests * Remove angular stuff * Update jest * Drag placeholder * Update events * Use old button styling and move css * Reviewing code * Add parameter rearrange test * Add Parameter Settings title change test * Update Parameter Settings button styling * Move parameter url logic back to Parameters * Disable url update when query is new * Styling changes (#4019) * Ran's title width styling * Update drag test * Improve sizing for Number inputs Co-Authored-By: Ran Byron <[email protected]> * Fix issue with dragged parameter wrapping Co-Authored-By: Ran Byron <[email protected]> * Don't reevaluate dirtyParamCount * Allow multiple values :) * Fix parameter alignments * Fix Select width on search * Update client/app/components/Parameters.less Co-Authored-By: Ran Byron <[email protected]> * Humanize param.name * Make sure angular updates Execute disabled status * Add more flake8 tests and fail build if any test fails (#4055) * Add more flake8 tests and fail build if any test fails Run all flake8 E9xx + F63x + F7xx + F82x tests. * long = long in Python 2 * Fix: MySQL connections without SSL are failing (#4090) * Move connection logic into a single method & make sure not to pass ssl value if not used. * Remove wildcard import and format file. * Removed redash-newstyle.less (#4017) * Make sure we always pass a list to _get_column_lists (#4095) (some data sources might return None as the columns list) * [Data Sources] Add: Azure Data Explorer (Kusto) query runner (#4091) * [Data Sources] Add: Azure Data Explorer (Kusto) query runner * CodeClimate fixes * Remove TODO * Fixed configuration properties names for Azure Kusto * Azure Kusto: get_schema in one query * azure-kusto-data update to 0.0.32 * Add Kusto to the default query runners list * [Qubole] - Adding support to process Quantum query types. (#4066) * [Qubole] - Adding support to process Quantum query types. Quantum is a serverless interactive service that offers direct SQL access to user's data lake. Changes are made to accept `quantum` query type from user which makes `Cluster Label` as optional. * -Making quantum as defult query. -Dictionary safe access to connection parmeters * keeping pep8 standards * Maintainig pep8 std * Use latest version of qds-sdk * Use qds-sdk v1.13.0 * Use qds-sdk v1.12.0 * Use qds-sdk v1.13.0 * Updating SDK with verified version * hive as default query type * qds-sdk : Locking most recent release version * qds-sdk : Locking recent release version * falling back to original version of qds-sdk * Dashboard: when updating parameters, run only relevant queries (#3804) * refresh only affected queries in dashboard when parameters are changed * rename pendingParameters to updatedParameters * select which widgets to update according to their mapping as a dashboard-level parameter * use lodash's include * Migrate with SQL statements. (#4105) * Update badge in README.md to link to CircleCI (#4104) * Update README.md * Update README.md * Update README.md Co-Authored-By: Ran Byron <[email protected]> * Update README.md * Fix widget bottom element alignment (#4110) * Fix: allow users with view only acces to use the queries in Query Results (#4112) * Fix: allow users with view only acces to access the queries * Add tests * Update error message * Update error message. Take 2 * Fix Dropdown parameter options appearing behind Dialog (#4109) * Add ability to use Ant's Table loading property when using ItemsTable (#4117) * Display data source icon in query editor (#4119) * Move annotation logic into Query Runner (#4113) * Code formatting * Move annotation logic into query runner, so it can be overriden in the query runner. * Add mixin to __all__ * Switch to flag instead of mixin * Feature (Redshift): option to set query group for adhoc/scheduled queries (#4114) * Add scheduled status to query job metadata. * Add: option to set query group for adhoc/scheduled Redshift queries * Scheduled might not be set for already enqueued queries. * Fix number param value normlization (#4116) * Use ng-src for data source icons (#4123) * hive_ds: show a user friendly error message when possible (#4121) * Update botocore, to get pass pip warning (#4122) * Widget table scroll-x visible (#4101) * Table viz horizontal scroll made visible * Added tests * Fixed snapshot pre-condition * Perhaps this would trigger percy * Upgrade Sentry-SDK and enable additional integratoins (#4127) * Update sentry-sdk version * Add additional Sentry integrations * Migrate Counter visualization to React (#4106) * Migrate Counter to React: Renderer * Migrate Counter to React: Editor * Cleanup * Review and fix rows indexing algorithm * Counter not properly scaled in editor * Fix wrong label for/input id pair * Tests * Tests * Fix vendor prefixes * Remove unnecessary useEffect dependencies * Update tests * Fix Percy snapshot names * Sync botocor eversions across requirements files. (#4128) * Decrease size of widget pagination (#4120) * Added tests * Perhaps this would trigger percy * Decrease size of widget pagination * Removed unused attr * Updated tests * Allow the user to decide how to handle null values in charts (#4071) * #2629 Refactor Chart visualization, add option for handling NULL values (keep/convert to 0.0) * Handle null values in line/area stacking code; some cleanup * Handle edge case: line/area stacking when last value of one of series is missing * Mjnor update to line/area stacking code * Fix line/area normalize to percents feature * Unit tests * Refine tests; add tests for prepareLayout function * Tests for prepareData (heatmap) function * Tests for prepareData (pie) function * Tests for prepareData (bar, line, area) function * Tests for prepareData (scatter, bubble) function * Tests for prepareData (box) function * Remove unused file * Alerts: Add more condition comparison options (#4134) * #4132 Add more condition comparison options * Add arguments to fallback lambda * Remove duplicate messages method (#4131) * Migrate Chart visualization to React Part 1: Renderer (#4130) * Migrate Chart visualization: Renderer * Refine PlotlyChart component; move stylesheets to visualization's folder * Migrate Custom JS Chart to React * Cleanup * Add jsconfig settings with '@' webpack alias (#4135) * Counter Editor: move components to own files (#4138) * Allow users to share aggregated usage information with us (#4108) * Initial commit of BeaconConsent component * Add comment about being able to change setting * Use <Text> correctly * Final version of consent screen * Show beacon consent message on homepage only if it wasn't enabled already. * Add consent setting to organization settings screen. * Add support for custom message in OrgSetting.save. * Implmenet consent saving. * If consent given, send extra data * Add HelpTrigger * Make CodeClimate happy * Wrap everything with DynamicComponent * CHANGELOG for V8-beta. (#4057) * CHANGELOG for V8-beta. * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md * Bug fix: Query view doesn't sync parameters when selecting and deleting (#4146) * Query Snippets: Use onClick instead of link for 'Click here' option (#4144) * Snippets: Don't change url when not needed * Revert "Snippets: Don't change url when not needed" This reverts commit 2f346f3. * Query Snippets: use onClick instead of link * Color picker component (#4136) * Widget filters overlapped by visualization (#4137) * Fix: widget filters overlapped by visualization * Fix tests * Fix tests * CHANGELOG for v8.0.0-beta.2 (#4145) * Stop building tarballs. * Update version reference. * CHANGELOG for 8.0.0-beta.2

* Migrate Chart visualization: Renderer * Refine PlotlyChart component; move stylesheets to visualization's folder * Migrate Custom JS Chart to React * Cleanup

Migrate Chart visualization: Renderer

73c2731

kravets-levko added Frontend Visualizations: Chart Visualizations Frontend: React Frontend codebase migration to React labels Sep 9, 2019

kravets-levko self-assigned this Sep 9, 2019

arikfr mentioned this pull request Sep 9, 2019

Migrate Visualizations to React #3301

Closed

24 tasks

Refine PlotlyChart component; move stylesheets to visualization's folder

ee47078

kravets-levko force-pushed the migrate-chart-visualization branch from 8ec6966 to ee47078 Compare September 10, 2019 10:29

kravets-levko commented Sep 10, 2019

View reviewed changes

kravets-levko added 2 commits September 10, 2019 20:34

Migrate Custom JS Chart to React

986aaad

Cleanup

308104d

kravets-levko requested review from arikfr, gabrieldutra and ranbena September 10, 2019 17:39

kravets-levko marked this pull request as ready for review September 10, 2019 17:39

ranbena reviewed Sep 11, 2019

View reviewed changes

ranbena approved these changes Sep 11, 2019

View reviewed changes

gabrieldutra approved these changes Sep 11, 2019

View reviewed changes

arikfr merged commit 54071e4 into master Sep 12, 2019

arikfr deleted the migrate-chart-visualization branch September 12, 2019 07:23

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

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

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 Chart visualization to React Part 1: Renderer #4130

Migrate Chart visualization to React Part 1: Renderer #4130

kravets-levko commented Sep 9, 2019 •

edited

Loading

arikfr commented Sep 9, 2019 via email

kravets-levko commented Sep 9, 2019

kravets-levko Sep 10, 2019

ranbena left a comment

ranbena Sep 11, 2019

kravets-levko Sep 11, 2019

ranbena Sep 11, 2019

kravets-levko Sep 11, 2019

arikfr commented Sep 12, 2019

Migrate Chart visualization to React Part 1: Renderer #4130

Migrate Chart visualization to React Part 1: Renderer #4130

Conversation

kravets-levko commented Sep 9, 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)

arikfr commented Sep 9, 2019 via email

kravets-levko commented Sep 9, 2019

kravets-levko Sep 10, 2019

Choose a reason for hiding this comment

ranbena left a comment

Choose a reason for hiding this comment

ranbena Sep 11, 2019

Choose a reason for hiding this comment

kravets-levko Sep 11, 2019

Choose a reason for hiding this comment

ranbena Sep 11, 2019

Choose a reason for hiding this comment

kravets-levko Sep 11, 2019

Choose a reason for hiding this comment

arikfr commented Sep 12, 2019

kravets-levko commented Sep 9, 2019 •

edited

Loading