This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405

Merged
n-riesco merged 21 commits into
plotly:masterfrom
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged

ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom
n-riesco:charteditor/use-react-chart-editor

Conversation

@n-riesco

@n-riescon-riesco commented Mar 13, 2018

Copy link
Copy Markdown
Contributor

@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).

peek 2018-03-13 17-45

@jackparmer

jackparmer commented Mar 13, 2018 via email

Copy link
Copy Markdown
Contributor

@nicolaskruchten

nicolaskruchten commented Mar 13, 2018

Copy link
Copy Markdown
Contributor

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

@n-riesco

n-riesco commented Mar 13, 2018

Copy link
Copy Markdown
ContributorAuthor

@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

jackparmer commented Mar 13, 2018 via email

Copy link
Copy Markdown
Contributor

@VeraZab

VeraZab commented Mar 13, 2018•
edited
Loading

Copy link
Copy Markdown

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

@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed the charteditor/use-react-chart-editor branch from d963467 to b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco

n-riesco commented Mar 15, 2018

Copy link
Copy Markdown
ContributorAuthor

@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?

peek 2018-03-15 20-05

@nicolaskruchten

nicolaskruchten commented Mar 15, 2018

Copy link
Copy Markdown
Contributor

react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!

@n-riesco

n-riesco commented Mar 16, 2018

Copy link
Copy Markdown
ContributorAuthor

@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.

@n-riesco

n-riesco commented Mar 16, 2018

Copy link
Copy Markdown
ContributorAuthor

@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).

peek 2018-03-16 21-12

@nicolaskruchten

nicolaskruchten commented Mar 19, 2018

Copy link
Copy Markdown
Contributor

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

jackparmer commented Mar 19, 2018

Copy link
Copy Markdown
Contributor

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

@n-riesco

n-riesco commented Mar 19, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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.

@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco

n-riesco commented Mar 20, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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.

@n-riescon-riesco mentioned this pull request Mar 21, 2018
Closed
@nicolaskruchten

nicolaskruchten commented Mar 21, 2018

Copy link
Copy Markdown
Contributor

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

@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco

n-riesco commented Mar 23, 2018

Copy link
Copy Markdown
ContributorAuthor

@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

n-riesco commented Mar 23, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@shannonlal

shannonlal commented Mar 24, 2018

Copy link
Copy Markdown
Contributor

@n-riesco Looking at this now

shannonlal
shannonlal reviewed Mar 24, 2018
View reviewed changes

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadapp/components/Settings/Preview/chart-editor.jsx Outdated
});

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

@shannonlalshannonlalMar 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadapp/components/Settings/Preview/chart-editor.jsx
<div
ref={'container'}
style={{
minHeight: 400,

@shannonlalshannonlalMar 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@n-riescon-riescoMar 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Comment threadapp/components/Settings/Settings.react.js
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';

@shannonlalshannonlalMar 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@n-riescon-riescoMar 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal reviewed Mar 24, 2018
View reviewed changes

@shannonlalshannonlal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃

Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx Outdated
columnNames = ['x', 'y'];

rows = [];
for (let i = 0; i < 100001; i++) {

@shannonlalshannonlalMar 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)

@n-riesco

n-riesco commented Mar 24, 2018

Copy link
Copy Markdown
ContributorAuthor

@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).

@n-riesco
n-riesco merged commit 3ffc80d into plotly:masterMar 24, 2018
@n-riescon-riesco changed the title [WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang

zhaodagang commented May 8, 2018

Copy link
Copy Markdown

Use ChartEditor, must connect plot site and logined ?

@n-riesco

n-riesco commented May 8, 2018

Copy link
Copy Markdown
ContributorAuthor

@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

zhaodagang commented May 9, 2018 via email

Copy link
Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405
Merged
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged
ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Conversation
@n-riesco
@n-riescon-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
@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).
peek 2018-03-13 17-45
@jackparmer
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@nicolaskruchten
nicolaskruchten
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
Awesome! New release coming soon with the new API /cc @VeraZab
@n-riesco
n-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@VeraZab
VeraZab
commented
Mar 13, 2018•

edited

Loading

Copy link


Copy Markdown

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor
@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed
the
charteditor/use-react-chart-editor
branch
from
d963467 to
b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco
n-riesco
commented
Mar 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@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?
peek 2018-03-15 20-05
@nicolaskruchten
nicolaskruchten
commented
Mar 15, 2018

Copy link


Copy Markdown

Contributor
react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
peek 2018-03-16 21-12
@nicolaskruchten
nicolaskruchten
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
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
jackparmer
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md
@n-riesco
n-riesco
commented
Mar 19, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco
n-riesco
commented
Mar 20, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riescon-riesco
mentioned this pull request
Mar 21, 2018
 Closed

@nicolaskruchten
nicolaskruchten
commented
Mar 21, 2018

Copy link


Copy Markdown

Contributor
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
@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@shannonlal I'm done with this PR. Would you review it, please?
@shannonlal
shannonlal
commented
Mar 24, 2018

Copy link


Copy Markdown

Contributor
@n-riesco Looking at this now
shannonlal
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadapp/components/Settings/Preview/chart-editor.jsx
Outdated

 });

 // Cap plots to 100k rows
 const length = Math.min(rows.length, 100000);
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be defined as a const at top. const MAX_ROWS=10000;
Comment threadapp/components/Settings/Preview/chart-editor.jsx
 <div
 ref={'container'}
 style={{
 minHeight: 400,
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Comment threadapp/components/Settings/Settings.react.js
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';
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃
Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx
Outdated

 columnNames = ['x', 'y'];

 rows = [];
 for (let i = 0; i < 100001; i++) {
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)
@n-riesco
n-riesco
commented
Mar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
@n-riesco
n-riesco
merged commit 3ffc80d
into
plotly:masterMar 24, 2018
@n-riescon-riesco
changed the title
[WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang
zhaodagang
commented
May 8, 2018

Copy link


Copy Markdown

Use ChartEditor, must connect plot site and logined ?
@n-riesco
n-riesco
commented
May 8, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
zhaodagang
commented
May 9, 2018
via email 

Copy link


Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405
Merged
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged
ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Conversation
@n-riesco
@n-riescon-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
@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).
peek 2018-03-13 17-45
@jackparmer
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@nicolaskruchten
nicolaskruchten
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
Awesome! New release coming soon with the new API /cc @VeraZab
@n-riesco
n-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@VeraZab
VeraZab
commented
Mar 13, 2018•

edited

Loading

Copy link


Copy Markdown

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor
@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed
the
charteditor/use-react-chart-editor
branch
from
d963467 to
b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco
n-riesco
commented
Mar 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@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?
peek 2018-03-15 20-05
@nicolaskruchten
nicolaskruchten
commented
Mar 15, 2018

Copy link


Copy Markdown

Contributor
react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
peek 2018-03-16 21-12
@nicolaskruchten
nicolaskruchten
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
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
jackparmer
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md
@n-riesco
n-riesco
commented
Mar 19, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco
n-riesco
commented
Mar 20, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riescon-riesco
mentioned this pull request
Mar 21, 2018
 Closed

@nicolaskruchten
nicolaskruchten
commented
Mar 21, 2018

Copy link


Copy Markdown

Contributor
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
@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@shannonlal I'm done with this PR. Would you review it, please?
@shannonlal
shannonlal
commented
Mar 24, 2018

Copy link


Copy Markdown

Contributor
@n-riesco Looking at this now
shannonlal
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadapp/components/Settings/Preview/chart-editor.jsx
Outdated

 });

 // Cap plots to 100k rows
 const length = Math.min(rows.length, 100000);
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be defined as a const at top. const MAX_ROWS=10000;
Comment threadapp/components/Settings/Preview/chart-editor.jsx
 <div
 ref={'container'}
 style={{
 minHeight: 400,
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Comment threadapp/components/Settings/Settings.react.js
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';
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃
Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx
Outdated

 columnNames = ['x', 'y'];

 rows = [];
 for (let i = 0; i < 100001; i++) {
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)
@n-riesco
n-riesco
commented
Mar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
@n-riesco
n-riesco
merged commit 3ffc80d
into
plotly:masterMar 24, 2018
@n-riescon-riesco
changed the title
[WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang
zhaodagang
commented
May 8, 2018

Copy link


Copy Markdown

Use ChartEditor, must connect plot site and logined ?
@n-riesco
n-riesco
commented
May 8, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
zhaodagang
commented
May 9, 2018
via email 

Copy link


Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405
Merged
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged
ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Conversation
@n-riesco
@n-riescon-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
@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).
peek 2018-03-13 17-45
@jackparmer
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@nicolaskruchten
nicolaskruchten
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
Awesome! New release coming soon with the new API /cc @VeraZab
@n-riesco
n-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@VeraZab
VeraZab
commented
Mar 13, 2018•

edited

Loading

Copy link


Copy Markdown

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor
@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed
the
charteditor/use-react-chart-editor
branch
from
d963467 to
b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco
n-riesco
commented
Mar 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@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?
peek 2018-03-15 20-05
@nicolaskruchten
nicolaskruchten
commented
Mar 15, 2018

Copy link


Copy Markdown

Contributor
react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
peek 2018-03-16 21-12
@nicolaskruchten
nicolaskruchten
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
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
jackparmer
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md
@n-riesco
n-riesco
commented
Mar 19, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco
n-riesco
commented
Mar 20, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riescon-riesco
mentioned this pull request
Mar 21, 2018
 Closed

@nicolaskruchten
nicolaskruchten
commented
Mar 21, 2018

Copy link


Copy Markdown

Contributor
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
@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@shannonlal I'm done with this PR. Would you review it, please?
@shannonlal
shannonlal
commented
Mar 24, 2018

Copy link


Copy Markdown

Contributor
@n-riesco Looking at this now
shannonlal
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadapp/components/Settings/Preview/chart-editor.jsx
Outdated

 });

 // Cap plots to 100k rows
 const length = Math.min(rows.length, 100000);
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be defined as a const at top. const MAX_ROWS=10000;
Comment threadapp/components/Settings/Preview/chart-editor.jsx
 <div
 ref={'container'}
 style={{
 minHeight: 400,
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Comment threadapp/components/Settings/Settings.react.js
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';
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃
Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx
Outdated

 columnNames = ['x', 'y'];

 rows = [];
 for (let i = 0; i < 100001; i++) {
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)
@n-riesco
n-riesco
commented
Mar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
@n-riesco
n-riesco
merged commit 3ffc80d
into
plotly:masterMar 24, 2018
@n-riescon-riesco
changed the title
[WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang
zhaodagang
commented
May 8, 2018

Copy link


Copy Markdown

Use ChartEditor, must connect plot site and logined ?
@n-riesco
n-riesco
commented
May 8, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
zhaodagang
commented
May 9, 2018
via email 

Copy link


Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405
Merged
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged
ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Conversation
@n-riesco
@n-riescon-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
@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).
peek 2018-03-13 17-45
@jackparmer
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@nicolaskruchten
nicolaskruchten
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
Awesome! New release coming soon with the new API /cc @VeraZab
@n-riesco
n-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@VeraZab
VeraZab
commented
Mar 13, 2018•

edited

Loading

Copy link


Copy Markdown

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor
@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed
the
charteditor/use-react-chart-editor
branch
from
d963467 to
b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco
n-riesco
commented
Mar 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@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?
peek 2018-03-15 20-05
@nicolaskruchten
nicolaskruchten
commented
Mar 15, 2018

Copy link


Copy Markdown

Contributor
react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
peek 2018-03-16 21-12
@nicolaskruchten
nicolaskruchten
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
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
jackparmer
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md
@n-riesco
n-riesco
commented
Mar 19, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco
n-riesco
commented
Mar 20, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riescon-riesco
mentioned this pull request
Mar 21, 2018
 Closed

@nicolaskruchten
nicolaskruchten
commented
Mar 21, 2018

Copy link


Copy Markdown

Contributor
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
@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@shannonlal I'm done with this PR. Would you review it, please?
@shannonlal
shannonlal
commented
Mar 24, 2018

Copy link


Copy Markdown

Contributor
@n-riesco Looking at this now
shannonlal
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadapp/components/Settings/Preview/chart-editor.jsx
Outdated

 });

 // Cap plots to 100k rows
 const length = Math.min(rows.length, 100000);
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be defined as a const at top. const MAX_ROWS=10000;
Comment threadapp/components/Settings/Preview/chart-editor.jsx
 <div
 ref={'container'}
 style={{
 minHeight: 400,
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Comment threadapp/components/Settings/Settings.react.js
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';
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃
Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx
Outdated

 columnNames = ['x', 'y'];

 rows = [];
 for (let i = 0; i < 100001; i++) {
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)
@n-riesco
n-riesco
commented
Mar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
@n-riesco
n-riesco
merged commit 3ffc80d
into
plotly:masterMar 24, 2018
@n-riescon-riesco
changed the title
[WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang
zhaodagang
commented
May 8, 2018

Copy link


Copy Markdown

Use ChartEditor, must connect plot site and logined ?
@n-riesco
n-riesco
commented
May 8, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
zhaodagang
commented
May 9, 2018
via email 

Copy link


Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405
Merged
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged
ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Conversation
@n-riesco
@n-riescon-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
@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).
peek 2018-03-13 17-45
@jackparmer
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@nicolaskruchten
nicolaskruchten
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
Awesome! New release coming soon with the new API /cc @VeraZab
@n-riesco
n-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@VeraZab
VeraZab
commented
Mar 13, 2018•

edited

Loading

Copy link


Copy Markdown

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor
@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed
the
charteditor/use-react-chart-editor
branch
from
d963467 to
b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco
n-riesco
commented
Mar 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@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?
peek 2018-03-15 20-05
@nicolaskruchten
nicolaskruchten
commented
Mar 15, 2018

Copy link


Copy Markdown

Contributor
react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
peek 2018-03-16 21-12
@nicolaskruchten
nicolaskruchten
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
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
jackparmer
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md
@n-riesco
n-riesco
commented
Mar 19, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco
n-riesco
commented
Mar 20, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riescon-riesco
mentioned this pull request
Mar 21, 2018
 Closed

@nicolaskruchten
nicolaskruchten
commented
Mar 21, 2018

Copy link


Copy Markdown

Contributor
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
@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@shannonlal I'm done with this PR. Would you review it, please?
@shannonlal
shannonlal
commented
Mar 24, 2018

Copy link


Copy Markdown

Contributor
@n-riesco Looking at this now
shannonlal
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadapp/components/Settings/Preview/chart-editor.jsx
Outdated

 });

 // Cap plots to 100k rows
 const length = Math.min(rows.length, 100000);
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be defined as a const at top. const MAX_ROWS=10000;
Comment threadapp/components/Settings/Preview/chart-editor.jsx
 <div
 ref={'container'}
 style={{
 minHeight: 400,
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Comment threadapp/components/Settings/Settings.react.js
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';
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃
Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx
Outdated

 columnNames = ['x', 'y'];

 rows = [];
 for (let i = 0; i < 100001; i++) {
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)
@n-riesco
n-riesco
commented
Mar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
@n-riesco
n-riesco
merged commit 3ffc80d
into
plotly:masterMar 24, 2018
@n-riescon-riesco
changed the title
[WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang
zhaodagang
commented
May 8, 2018

Copy link


Copy Markdown

Use ChartEditor, must connect plot site and logined ?
@n-riesco
n-riesco
commented
May 8, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
zhaodagang
commented
May 9, 2018
via email 

Copy link


Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405
Merged
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged
ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Conversation
@n-riesco
@n-riescon-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
@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).
peek 2018-03-13 17-45
@jackparmer
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@nicolaskruchten
nicolaskruchten
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
Awesome! New release coming soon with the new API /cc @VeraZab
@n-riesco
n-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@VeraZab
VeraZab
commented
Mar 13, 2018•

edited

Loading

Copy link


Copy Markdown

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor
@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed
the
charteditor/use-react-chart-editor
branch
from
d963467 to
b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco
n-riesco
commented
Mar 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@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?
peek 2018-03-15 20-05
@nicolaskruchten
nicolaskruchten
commented
Mar 15, 2018

Copy link


Copy Markdown

Contributor
react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
peek 2018-03-16 21-12
@nicolaskruchten
nicolaskruchten
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
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
jackparmer
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md
@n-riesco
n-riesco
commented
Mar 19, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco
n-riesco
commented
Mar 20, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riescon-riesco
mentioned this pull request
Mar 21, 2018
 Closed

@nicolaskruchten
nicolaskruchten
commented
Mar 21, 2018

Copy link


Copy Markdown

Contributor
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
@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@shannonlal I'm done with this PR. Would you review it, please?
@shannonlal
shannonlal
commented
Mar 24, 2018

Copy link


Copy Markdown

Contributor
@n-riesco Looking at this now
shannonlal
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadapp/components/Settings/Preview/chart-editor.jsx
Outdated

 });

 // Cap plots to 100k rows
 const length = Math.min(rows.length, 100000);
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be defined as a const at top. const MAX_ROWS=10000;
Comment threadapp/components/Settings/Preview/chart-editor.jsx
 <div
 ref={'container'}
 style={{
 minHeight: 400,
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Comment threadapp/components/Settings/Settings.react.js
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';
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃
Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx
Outdated

 columnNames = ['x', 'y'];

 rows = [];
 for (let i = 0; i < 100001; i++) {
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)
@n-riesco
n-riesco
commented
Mar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
@n-riesco
n-riesco
merged commit 3ffc80d
into
plotly:masterMar 24, 2018
@n-riescon-riesco
changed the title
[WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang
zhaodagang
commented
May 8, 2018

Copy link


Copy Markdown

Use ChartEditor, must connect plot site and logined ?
@n-riesco
n-riesco
commented
May 8, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
zhaodagang
commented
May 9, 2018
via email 

Copy link


Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content

This repository was archived by the owner on Aug 29, 2025. It is now read-only.

ChartEditor: use react-chart-editor - #405
Merged
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Mar 24, 2018
Merged
ChartEditor: use react-chart-editor#405
n-riesco merged 21 commits into
plotly:masterfrom 
n-riesco:charteditor/use-react-chart-editor
Conversation
@n-riesco
@n-riescon-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
@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).
peek 2018-03-13 17-45
@jackparmer
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@nicolaskruchten
nicolaskruchten
commented
Mar 13, 2018

Copy link


Copy Markdown

Contributor
Awesome! New release coming soon with the new API /cc @VeraZab
@n-riesco
n-riesco
commented
Mar 13, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
jackparmer
commented
Mar 13, 2018
via email 

Copy link


Copy Markdown

Contributor
@VeraZab
VeraZab
commented
Mar 13, 2018•

edited

Loading

Copy link


Copy Markdown

@n-riesco just made a new release: https://www.npmjs.com/package/react-chart-editor
@n-riesco
ChartEditor: new implementation
4dc50d6
* Implemented ChartEditor using `react-chart-editor@0.11`.
* Enabled bundling of CSS by importing from a react component.
* Use table view by default (instead of ChartEditor).
Closesplotly#350
@n-riesco
ChartEditor: remove old implementation
01383be
@n-riesco
ChartEditor: store gd in Settings state
e93e8fb
* Store gd in Settings state so that we can export the chart into Chart
Studio.
@n-riesco
ChartEditor: remove obsolete props and state
b5624fe
* Removed obsolete props and state in Preview used by previous
implementation of ChartEditor.
@n-riesco
n-riescoforce-pushed
the
charteditor/use-react-chart-editor
branch
from
d963467 to
b5624feCompareMarch 15, 2018 13:24
@n-riesco
ChartEditor: upgrade to react-chart-editor@0.12
8b03bbb
@n-riesco
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.
@n-riesco
Preview: hide code editor and schemas
c99966f
* Hide code editor and schemas view, when the Chart Editor is selected.
@n-riesco
n-riesco
commented
Mar 15, 2018

Copy link


Copy Markdown

ContributorAuthor
@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?
peek 2018-03-15 20-05
@nicolaskruchten
nicolaskruchten
commented
Mar 15, 2018

Copy link


Copy Markdown

Contributor
react-plotly.js@2.1.0 and react-chart-editor@0.13.0 should resolve the sizing issue!
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@nicolaskruchten 😿 I still see the same behaviour after upgrading to react-plotly.js@2.1.0 and react-chart-editor@0.13.0.
@n-riesco
n-riesco
commented
Mar 16, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
peek 2018-03-16 21-12
@nicolaskruchten
nicolaskruchten
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
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
jackparmer
commented
Mar 19, 2018

Copy link


Copy Markdown

Contributor
@nicolaskruchten This works for me: https://github.com/plotly/falcon-sql-client/blob/master/CONTRIBUTING.md
@n-riesco
n-riesco
commented
Mar 19, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riesco
ChartEditor: mount PlotlyEditor only if visible
85afe3b
* Fixes PlotlyComponent autosizing issue.
@n-riesco
ChartEditor: improve performance
5f9d4fa
* Detect before render when chart panel becomes visible.
@n-riesco
n-riesco
commented
Mar 20, 2018•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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.
@n-riescon-riesco
mentioned this pull request
Mar 21, 2018
 Closed

@nicolaskruchten
nicolaskruchten
commented
Mar 21, 2018

Copy link


Copy Markdown

Contributor
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
@n-riesco
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).
Fixesplotly#395
@n-riesco
Preview: prevent selection of text in tab headers
8924044
@n-riesco
ChartEditor: do not plot more than 100k rows
10cffc3
@n-riesco
ChartEditor: add tests
04dd246
@n-riesco
ChartEditor: rename gd to plotlyJSON
3f836fb
* Restored previous name, because `gd` could suggest it's the DOM
element.
* `plotlyJSON` can be stringified safely.
@n-riesco
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
n-riesco
commented
Mar 23, 2018

Copy link


Copy Markdown

ContributorAuthor
@shannonlal I'm done with this PR. Would you review it, please?
@shannonlal
shannonlal
commented
Mar 24, 2018

Copy link


Copy Markdown

Contributor
@n-riesco Looking at this now
shannonlal
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment threadapp/components/Settings/Preview/chart-editor.jsx
Outdated

 });

 // Cap plots to 100k rows
 const length = Math.min(rows.length, 100000);
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be defined as a const at top. const MAX_ROWS=10000;
Comment threadapp/components/Settings/Preview/chart-editor.jsx
 <div
 ref={'container'}
 style={{
 minHeight: 400,
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Comment threadapp/components/Settings/Settings.react.js
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';
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@n-riescon-riescoMar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
shannonlal
reviewed
Mar 24, 2018
View reviewed changes
@shannonlalshannonlal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 💃
Comment threadtest/app/components/Settings/Preview/chart-editor.test.jsx
Outdated

 columnNames = ['x', 'y'];

 rows = [];
 for (let i = 0; i < 100001; i++) {
@shannonlalshannonlalMar 24, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be (MAX_LENGTH + 1)
@n-riesco
n-riesco
commented
Mar 24, 2018

Copy link


Copy Markdown

ContributorAuthor
@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).
@n-riesco
n-riesco
merged commit 3ffc80d
into
plotly:masterMar 24, 2018
@n-riescon-riesco
changed the title
[WIP] ChartEditor: use react-chart-editorChartEditor: use react-chart-editorMar 24, 2018
@zhaodagang
zhaodagang
commented
May 8, 2018

Copy link


Copy Markdown

Use ChartEditor, must connect plot site and logined ?
@n-riesco
n-riesco
commented
May 8, 2018

Copy link


Copy Markdown

ContributorAuthor
@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
zhaodagang
commented
May 9, 2018
via email 

Copy link


Copy Markdown

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@shannonlalshannonlalshannonlal left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@n-riesco@jackparmer@nicolaskruchten@VeraZab@shannonlal@zhaodagang