ChartEditor: use react-chart-editor #405

n-riesco · 2018-03-13T17:51:37Z

@jackparmer This is just to show you how Falcon would look like with react-chart-editor.

See that I've made the Table view the default.

I'm also thinking of making the tree view as big as the code editor. So that the chart editor can use the whole width.

@nicolaskruchten I'm looking forward to the new API (it'd simplify this PR a lot).

jackparmer · 2018-03-13T18:00:05Z

Nice. It looks great. When you switch to the chart tab, should we auto-minimize the table schema? `react-chart-editor` needs a good amount of horizontal space and users might not now that they can collapse the table schema sidebar.

…

On Tue, Mar 13, 2018 at 1:51 PM, Nicolas Riesco ***@***.***> wrote: @jackparmer <https://github.com/jackparmer> This is just to show you how Falcon would look like with react-chart-editor. See that I've made the Table view the default. I'm also thinking of making the tree view as big as the code editor. So that the chart editor can use the whole width. @nicolaskruchten <https://github.com/nicolaskruchten> I'm looking forward to the new API (it'd simplify this PR a lot). [image: peek 2018-03-13 17-45] <https://user-images.githubusercontent.com/6199391/37359812-782f9a1c-26e6-11e8-9b3f-e48275e62271.gif> ------------------------------ You can view, comment on, or merge this pull request online at: #405 Commit Summary - ChartEditor: WIP File Changes - *M* app/components/Settings/Preview/Preview.react.js <https://github.com/plotly/falcon-sql-client/pull/405/files#diff-0> (24) - *A* app/components/Settings/Preview/chart-editor.jsx <https://github.com/plotly/falcon-sql-client/pull/405/files#diff-1> (134) - *M* package.json <https://github.com/plotly/falcon-sql-client/pull/405/files#diff-2> (6) - *M* webpack.config.base.js <https://github.com/plotly/falcon-sql-client/pull/405/files#diff-3> (7) - *M* yarn.lock <https://github.com/plotly/falcon-sql-client/pull/405/files#diff-4> (1015) Patch Links: - https://github.com/plotly/falcon-sql-client/pull/405.patch - https://github.com/plotly/falcon-sql-client/pull/405.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#405>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABx4apFPzoyZIzzO4peWgRudX5EiAO8Bks5teAcqgaJpZM4SpKgR> .

nicolaskruchten · 2018-03-13T18:03:59Z

Awesome! New release coming soon with the new API /cc @VeraZab

n-riesco · 2018-03-13T18:10:54Z

@jackparmer I thought about autominimise, but I was concerned some users might find difficult to discover that they can make the schema view visible again. That's why I thought making the schema view as big as the code editor would be the solution.

jackparmer · 2018-03-13T18:43:21Z

That's why I thought making the schema view as big as the code editor

would be the solution. Yeah, that sounds good to me.

…

On Tue, Mar 13, 2018 at 2:10 PM, Nicolas Riesco ***@***.***> wrote: @jackparmer <https://github.com/jackparmer> I thought about autominimise, but I was concerned some users might find difficult to discover that they can make the schema view visible again. That's why I thought making the schema view as big as the code editor would be the solution. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#405 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABx4ajQCXkpwdfd467hxHfFGJ06oBpm_ks5teAuwgaJpZM4SpKgR> .

VeraZab · 2018-03-13T18:47:40Z

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor

* Implemented ChartEditor using `[email protected]`. * Enabled bundling of CSS by importing from a react component. * Use table view by default (instead of ChartEditor). Closes plotly#350

* Store gd in Settings state so that we can export the chart into Chart Studio.

* Removed obsolete props and state in Preview used by previous implementation of ChartEditor.

* Moved TableTree from Settings into Preview, so that all the components under the Query panel live inside Preview. * After this change, it'll be possible for move ChartEditor so that it fills all the available width.

* Hide code editor and schemas view, when the Chart Editor is selected.

n-riesco · 2018-03-15T20:17:32Z

@jackparmer I've updated the PR. In this end I've gone for the option of hiding both the code editor and the table schemas view (it didn't make sense to hide the code editor while the table schemas view was still visible)

@nicolaskruchten @VeraZab As you can see in the screencast, I've got a problem with the initial plot size. Is there a way to set this size with the new API?

nicolaskruchten · 2018-03-15T23:04:29Z

[email protected] and [email protected] should resolve the sizing issue!

* Upgraded to `[email protected]` and `[email protected]`.

n-riesco · 2018-03-16T19:24:34Z

@nicolaskruchten 😿 I still see the same behaviour after upgrading to [email protected] and [email protected].

n-riesco · 2018-03-16T21:22:29Z

@nicolaskruchten One thing, that is rather inconvenient with this workaround, is that I have to hard-code the height as style={{height: 400}}. I tried style={{minHeight: 400, height: '100%'}}, but it wouldn't work (the height of <PlotlyEditor> would be less than 400px).

nicolaskruchten · 2018-03-19T13:24:14Z

What's the easiest way for me to get this running locally so I can play with the CSS? The height/width thing is a big pain point with this project in general :(

jackparmer · 2018-03-19T20:32:49Z

@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md

n-riesco · 2018-03-19T22:20:42Z

@nicolaskruchten It turns out the issue isn't with the CSS. In fact, style={{minHeight: 400, width: '100%'}} works if I'm careful to mount <PlotEditor> only when the parent node is visible (otherwise this.PlotComponent throws errors when window.getComputedStyle(parentNode).display === 'none).

I have something working, but I'll clean it and push it into the PR tomorrow.

* Fixes PlotlyComponent autosizing issue.

* Detect before render when chart panel becomes visible.

n-riesco · 2018-03-20T17:27:46Z

@nicolaskruchten In principle 85afe3b could be implemented inside react-chart-editor, but this solution isn't very efficient.

I think the place to deal with this issue is inside plotly.js (I don't know what the implications are, but ideally we should be able to tell plotly.js to ignore display: none; and draw the chart as if it was visible).

@etpinard @alexcjohnson ⬆️

To sum up the issue:

the issue is that <PlotlyComponent>'s initial size doesn't honour the bounding client rect.
the issue happens when <PlotlyComponent> is mounted, because React creates children before their parents.
and it also happens when <PlotlyComponent> is resized and <PlotlyComponent> isn't visible.

nicolaskruchten · 2018-03-21T13:01:40Z

Seconded, this is a problem in the editor in general but I thought I'd more or less fixed it by firing window.resize after drawing. Clearly that's not late enough in the React loop to catch all cases. It's happening in Dash-land as well: https://plotly.slack.com/archives/C90K5TYP3/p1521599318000139

* Preview.componentWillReceiveProps now checks if props have change before triggering a state update. * Do not pre-compute the CSV string (in order to reduce memory usage). Fixes plotly#395

* Restored previous name, because `gd` could suggest it's the DOM element. * `plotlyJSON` can be stringified safely.

n-riesco · 2018-03-23T21:19:31Z

@nicolaskruchten thanks for reviewing https://github.com/n-riesco/plotly-database-connector/blob/04dd2463ccddf834b19ef4111cffa989bf7f7fa6/app/components/Settings/Preview/chart-editor.jsx . I've renamed gd back to plotlyJSON to make clear this isn't the DOM element.

n-riesco · 2018-03-23T21:55:13Z

@shannonlal I'm done with this PR. Would you review it, please?

shannonlal · 2018-03-24T01:41:43Z

@n-riesco Looking at this now

shannonlal

Your throughts on define a standard for file name convention? Maybe out it in the contributing doc? I noticed that as you have be updating files you have been moving away from camel case file names to dash. I am okay with this, I just want to know if we should standardize on this. We updated chart-editor.css and Preview.css. I wanted to know if we should be making them consistent

shannonlal · 2018-03-24T01:43:17Z

app/components/Settings/Preview/chart-editor.jsx

+        });
+
+        // Cap plots to 100k rows
+        const length = Math.min(rows.length, 100000);


This should probably be defined as a const at top. const MAX_ROWS=10000;

shannonlal · 2018-03-24T01:45:06Z

app/components/Settings/Preview/chart-editor.jsx

+            <div
+                ref={'container'}
+                style={{
+                    minHeight: 400,


Just a suggestion. Maybe a constant or move to a separate file. I see the min height of 400 defined in a bunch of files. Maybe we should centralize to a single file? your thoughts?

I've re-enabled css-loader, so that we can import again .css files into a component. But I didn't create a chart-editor.css, because I feel we need tidy up the CSS in Falcon first:

perhaps define a .css file with a common theme

follow Plotly's style guide

Jack mentioned tachyons and I'm keen to give it a go.

shannonlal · 2018-03-24T01:46:26Z

app/components/Settings/Settings.react.js

 import {Tab, Tabs, TabList, TabPanel} from 'react-tabs';
 import ConnectionTabs from './Tabs/Tabs.react';
 import UserConnections from './UserConnections/UserConnections.react';
 import DialectSelector from './DialectSelector/DialectSelector.react';
 import ConnectButton from './ConnectButton/ConnectButton.react';
 import Preview from './Preview/Preview.react';
-import TableTree from './Preview/TableTree.react.js';


Is this file no longer in use? I was working on some jest tests for table tree. If it is not needed I can move on to some other tests

yes, it's still in use (tests are always welcome 😄). I moved the import to Preview.react.js. Now Preview.react.js contains all the components displayed under the QUERY tab.

shannonlal

Nothing major. Just some minor comments and more questions about standards. I am going to give this a dancer now and let you make decision as to wether you want to fix my comments or move them to another issue. Overall a really nice job. 💃

shannonlal · 2018-03-24T01:50:19Z

test/app/components/Settings/Preview/chart-editor.test.jsx

+        columnNames = ['x', 'y'];
+
+        rows = [];
+        for (let i = 0; i < 100001; i++) {


Should this be (MAX_LENGTH + 1)

n-riesco · 2018-03-24T16:29:36Z

@shannonlal I've addressed all your comments except that about CSS (I'd rather address it after we devise a plan to tidy up the CSS in Falcon; see what I replied here).

zhaodagang · 2018-05-08T11:24:11Z

Use ChartEditor, must connect plot site and logined ?

n-riesco · 2018-05-08T15:43:34Z

@zhaodagang Falcon and react-chart-editor can be used without login.

react-chart-editor is based on the open-source library plotly.js.

If you create an account on https://plot.ly/ and login with Falcon, then you'll be able to upload your data and charts onto https://plot.ly/ .

zhaodagang · 2018-05-09T02:17:03Z

Can this App connect clickhouse database ? 2018-05-09 zdg8882000 发件人：Nicolas Riesco <[email protected]> 发送时间：2018-05-08 23:43 主题：Re: [plotly/falcon-sql-client] ChartEditor: use react-chart-editor (#405) 收件人："plotly/falcon-sql-client"<[email protected]> 抄送："Mr Zhao"<[email protected]>,"Mention"<[email protected]> @zhaodagang Falcon and react-chart-editor can be used without login. react-chart-editor is based on the open-source library plotly.js. If you create an account on https://plot.ly/ and login with Falcon, then you'll be able to upload your data and charts onto https://plot.ly/ . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

n-riesco added 4 commits March 15, 2018 11:25

ChartEditor: new implementation

4dc50d6

* Implemented ChartEditor using `[email protected]`. * Enabled bundling of CSS by importing from a react component. * Use table view by default (instead of ChartEditor). Closes plotly#350

ChartEditor: remove old implementation

01383be

ChartEditor: store gd in Settings state

e93e8fb

* Store gd in Settings state so that we can export the chart into Chart Studio.

ChartEditor: remove obsolete props and state

b5624fe

* Removed obsolete props and state in Preview used by previous implementation of ChartEditor.

n-riesco force-pushed the charteditor/use-react-chart-editor branch from d963467 to b5624fe Compare March 15, 2018 13:24

n-riesco added 3 commits March 15, 2018 16:50

ChartEditor: upgrade to [email protected]

8b03bbb

move TableTree from Settings into Preview

a8cdf78

* Moved TableTree from Settings into Preview, so that all the components under the Query panel live inside Preview. * After this change, it'll be possible for move ChartEditor so that it fills all the available width.

Preview: hide code editor and schemas

c99966f

* Hide code editor and schemas view, when the Chart Editor is selected.

n-riesco added 4 commits March 16, 2018 17:58

static: remove unused css files

bb2ea2b

Preview: remove unused styles

fc0833d

Settings: remove unused styles

85e89d0

ChartEditor: upgrade to [email protected]

44374c5

* Upgraded to `[email protected]` and `[email protected]`.

ChartEditor: workaround to fix initial chart size

61e2e46

n-riesco added 2 commits March 20, 2018 17:07

ChartEditor: mount PlotlyEditor only if visible

85afe3b

* Fixes PlotlyComponent autosizing issue.

ChartEditor: improve performance

5f9d4fa

* Detect before render when chart panel becomes visible.

n-riesco mentioned this pull request Mar 21, 2018

Prevent negative width or height on rect plotly/plotly.js#2482

Closed

n-riesco added 5 commits March 23, 2018 15:51

Preview: prevent unnecessary state updates

84e710a

* Preview.componentWillReceiveProps now checks if props have change before triggering a state update. * Do not pre-compute the CSV string (in order to reduce memory usage). Fixes plotly#395

Preview: prevent selection of text in tab headers

8924044

ChartEditor: do not plot more than 100k rows

10cffc3

ChartEditor: add tests

04dd246

ChartEditor: rename gd to plotlyJSON

3f836fb

* Restored previous name, because `gd` could suggest it's the DOM element. * `plotlyJSON` can be stringified safely.

shannonlal reviewed Mar 24, 2018

View reviewed changes

n-riesco added 2 commits March 24, 2018 15:11

ChartEditor: define MAX_ROWS

9388445

add CODING_GUIDELINES.md

cc5ec49

n-riesco merged commit 3ffc80d into plotly:master Mar 24, 2018

n-riesco changed the title ~~[WIP] ChartEditor: use react-chart-editor~~ ChartEditor: use react-chart-editor Mar 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ChartEditor: use react-chart-editor #405

ChartEditor: use react-chart-editor #405

n-riesco commented Mar 13, 2018

jackparmer commented Mar 13, 2018 via email

nicolaskruchten commented Mar 13, 2018

n-riesco commented Mar 13, 2018

jackparmer commented Mar 13, 2018 via email

VeraZab commented Mar 13, 2018 •

edited

Loading

n-riesco commented Mar 15, 2018

nicolaskruchten commented Mar 15, 2018

n-riesco commented Mar 16, 2018

n-riesco commented Mar 16, 2018

nicolaskruchten commented Mar 19, 2018

jackparmer commented Mar 19, 2018

n-riesco commented Mar 19, 2018 •

edited

Loading

n-riesco commented Mar 20, 2018 •

edited

Loading

nicolaskruchten commented Mar 21, 2018

n-riesco commented Mar 23, 2018

n-riesco commented Mar 23, 2018

shannonlal commented Mar 24, 2018

shannonlal left a comment

shannonlal Mar 24, 2018

shannonlal Mar 24, 2018

n-riesco Mar 24, 2018

shannonlal Mar 24, 2018

n-riesco Mar 24, 2018

shannonlal left a comment

shannonlal Mar 24, 2018

n-riesco commented Mar 24, 2018

zhaodagang commented May 8, 2018

n-riesco commented May 8, 2018

zhaodagang commented May 9, 2018 via email

ChartEditor: use react-chart-editor #405

ChartEditor: use react-chart-editor #405

Conversation

n-riesco commented Mar 13, 2018

jackparmer commented Mar 13, 2018 via email

nicolaskruchten commented Mar 13, 2018

n-riesco commented Mar 13, 2018

jackparmer commented Mar 13, 2018 via email

VeraZab commented Mar 13, 2018 • edited Loading

n-riesco commented Mar 15, 2018

nicolaskruchten commented Mar 15, 2018

n-riesco commented Mar 16, 2018

n-riesco commented Mar 16, 2018

nicolaskruchten commented Mar 19, 2018

jackparmer commented Mar 19, 2018

n-riesco commented Mar 19, 2018 • edited Loading

n-riesco commented Mar 20, 2018 • edited Loading

nicolaskruchten commented Mar 21, 2018

n-riesco commented Mar 23, 2018

n-riesco commented Mar 23, 2018

shannonlal commented Mar 24, 2018

shannonlal left a comment

Choose a reason for hiding this comment

shannonlal Mar 24, 2018

Choose a reason for hiding this comment

shannonlal Mar 24, 2018

Choose a reason for hiding this comment

n-riesco Mar 24, 2018

Choose a reason for hiding this comment

shannonlal Mar 24, 2018

Choose a reason for hiding this comment

n-riesco Mar 24, 2018

Choose a reason for hiding this comment

shannonlal left a comment

Choose a reason for hiding this comment

shannonlal Mar 24, 2018

Choose a reason for hiding this comment

n-riesco commented Mar 24, 2018

zhaodagang commented May 8, 2018

n-riesco commented May 8, 2018

zhaodagang commented May 9, 2018 via email

VeraZab commented Mar 13, 2018 •

edited

Loading

n-riesco commented Mar 19, 2018 •

edited

Loading

n-riesco commented Mar 20, 2018 •

edited

Loading