Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit Plotly integration #2117

Closed
arikfr opened this issue Dec 4, 2017 · 11 comments
Closed

Revisit Plotly integration #2117

arikfr opened this issue Dec 4, 2017 · 11 comments

Comments

@arikfr
Copy link
Member

arikfr commented Dec 4, 2017

Our Plotly integration contains a lot of custom code, mainly around stacking. It created some issues over the years (#1902, #1832, #1326, #2116). We also miss out on new features like annotations (#1254).

Let's revisit the integration code, clean it up and mainly:

  • remove our custom stacking code (if possible).
  • add support for annotations.

Once this is done, we can start adding new features (like #728 for example).

@deecay
Copy link
Contributor

deecay commented Dec 4, 2017

I would like to add

  • Supporting graph height/width and margins.

as one very basic yet important point. Related controls and codes are scattered in couple of places and this is a good opportunity to review what should be done for graph heights and margins.

@arikfr
Copy link
Member Author

arikfr commented Dec 5, 2017

Height can be changed by adjusting the widget height now. And why would someone need to change the margins?

@deecay
Copy link
Contributor

deecay commented Dec 5, 2017

Is height adjustable in query page?
Lets say an author created bar chart and wants it to be 1000px high. Does he have to put it in dashboard to see if the query and chart config are just right?

I will post screenshots later for margins.

@kravets-levko
Copy link
Collaborator

Hi @deecay! Your idea definitely makes sense, but, IMHO, we should think a step forward:

  1. can this use case be applied to another visualizations? I think - yes.
  2. should that width/height option be applied in other places - not only on query page, but in editor's preview, shared dashboard, shared viz, etc.? Definitely yes, for a common behavior.

We already talked with @arikfr about making embedded visualizations to fit size of <iframe> (in the same manner as they fit widgets in dashboard). So, probably we should work out this idea and apply it to query page somehow. WDYT?

@arikfr
Copy link
Member Author

arikfr commented Dec 5, 2017

To expand on @kravets-levko's comment: I think that we need to make visualization take the space they are given. We already have this behavior on dashboard widgets, we just need to expand it to embeds and query page too.

Then to accommodate your use case we can make the visualization section on a query page resizable.

The only question remains is whether the size of the visualization section should be persisted with the visualization/query. For the sake of simplicity, I would go with no persistence, and revisit this decision after we have this "in the wild".

WDYT?

@deecay
Copy link
Contributor

deecay commented Dec 6, 2017

Hi,
Now I understand that there is a plan that "fit height to container" is going to be applied to places other than dashboards. That is awesome.

Then I just wanted to point out query page is important to consider, and I agree with @kravets-levko that there are even more places to consider, such as editor's preview.

@arikfr, by non-persistence, you mean not saved anywhere? For some users dashboard is an overkill, and only need one query page and related visualizations to get their jobs done. For those cases I give out the query page URL. "Hide Source" is a useful feature here, the query page can be a one-topic pseudo-dashboard. So I think it's better to save the height of query-page-viz that has been decided by the author. Height of editor preview can be discarded.

BTW, this discussion reminded me that I thought counter visualization in query page is a little too small. After clicking the widget header of a large counter-viz, I couldn't find the counter viz in query page because of the stark size difference. This could be solved with the above idea, or just a larger minimum height.

  • Margin problem. I could only find these two cases.
    snapcrab_noname_2017-12-6_16-35-28_no-00
    Also, please note we will have to consider top margins when adding value annotation feature, for there is no space to place value annotations now (bar with largest value occupies 100% up to the "ceiling").

@arikfr
Copy link
Member Author

arikfr commented Dec 6, 2017

Height: but if the visualization consumed 100% of available space, wouldn't it be enough? I mean on a results page (source hidden), there will usually be plenty of vertical space for a chart. On some small resolutions it might be a problem, and the chart might not be as big as the author might want, but this feels like an edge case.

Margins: I really wish we (or Plotly) had a solution for this that doesn't require manual intervention. It feels like something "the computer" should do and not the user :) When we revisit the implementation, need to check if they added some auto sizing features.

@deecay
Copy link
Contributor

deecay commented Dec 6, 2017

  • On Height
    I agree that a very tall chart is now an edge case. Let me throw in yet another aspect, subplots. I've had one case where I wanted to subplot one big scatter chart in to 3x3 (therefore total of 9) subplots. Currently , grouping can only be done within the chart, but there was a case like that. Pie charts have fixed 2-row configuration for subplotting, this this could expand in the future, with configurable row/column number.
    I think pretty much every chart type can have decent use cases for subplots, which makes the height taller. WDYT?

  • On Margins
    I totally agree. This is something that should be handled by program.

@deecay
Copy link
Contributor

deecay commented Dec 13, 2017

Past discussion on auto-margin calculation in Plotly.js issues.

@arikfr arikfr closed this as completed Jan 29, 2018
@deecay
Copy link
Contributor

deecay commented Mar 2, 2018

FYI, plotly v1.35.0 will have auto-margin for long labels. See plotly/plotly.js#2243 .

@arikfr
Copy link
Member Author

arikfr commented Mar 4, 2018

Thanks for the heads up, @deecay !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants