Uh oh!
There was an error while loading. Please reload this page.
Plotting in child window - #764
Conversation
- Graph divs now keep a reference to their 'parent' document in the `_document' property. - Styles are now injected to the parent document on the first call to Plotly.plot. - Anything that draws over a document for interactivity purposes (eg. dragcovers), now uses the _document reference to ensure it is drawn in the correct document. - Notifiers now require a graph div as the first argument so that notifications are rendered in the correct place
| 'use strict'; | ||
| // expands a plotcss selector | ||
| exports.buildFullSelector = function buildFullSelector(selector) { |
There was a problem hiding this comment.
On second thought, would you mind moving this file and src/css/plotcss_injector.js to src/lib/. I would prefer leaving the src/css/ directory for css (and scss) files only.
I would also vote for merging the two files into one and naming it something like plotcss_utils.js
Sorry for the confusion.
etpinard
commented
Jul 21, 2016
Looks like something went wrong when I squashed your commits. Now the linter is complaining:
|
| }); | ||
| }); | ||
| describe('css injection', function() { |
There was a problem hiding this comment.
Ha here's why the linter is failing. Simply removing this block (which is duplicated in plot_css_test.js, correct?) should do the trick.
This keeps the css directory free of Javascript. The helpers have been moved inside the plotcss_utils.js file instead of a separate helpers file. The plot API and tests have been updated accordingly. Removed duplicate tests in plot_interact_tests.js
nielsenb-jf
commented
Jul 21, 2016
Sorry about the duplicate tests. Think everything is cleaned up now. |
| for(var i = 0; i < sourceDocument.styleSheets.length; i++) { | ||
| var styleSheet = sourceDocument.styleSheets[i]; | ||
| for(var j = 0; j < styleSheet.cssRules.length; j++) { |
There was a problem hiding this comment.
@nielsenb-jf looks like sytleSheet.cssRules can be undefined in some scenarios (e.g. in our npm start test dashboard). This leads to an exception here.
Adding
if(!styleSheet.cssRules)continue;above or subbing in
for(varj=0;j<(styleSheet.cssRules||[]).length;j++){should do the trick.
It's possible for a stylesheet's cssRules property to be undefined. In that case, break out of the rule aggregating loop. Don't explicitly refer to module.exports.
nielsenb-jf
commented
Jul 22, 2016
Good catch! Fixed. |
etpinard
commented
Jul 25, 2016
Great. Looking good 🎉 I'll give a final spin at some point later this week (I'll be offline for the next two days). Thanks again for your contribution. |
etpinard
commented
Jul 28, 2016
@nielsenb-jf amazing PR. Thank you very much for your efforts. 🍻 |
This pull request fixes the issues noted in issue 702. Essentially, if Plotly was provided an element in a different window or tab, the plot would not be styled correctly and interactivity (hovers, drag zoom, etc.) would not work.
It is squished to one commit due to issues caused by multiple merges during development see this previous PR.
The following are the big changes to fix this issue:
Plotly.plot._documentreference to ensure it is drawn in the correct document.