plotly / Plotly.NET

interactive graphing library for .NET programming languages :chart_with_upwards_trend:
https://plotly.net
MIT License
654 stars 85 forks source link

Charts marked as `Responsive = true` are not responsive #448

Closed johnW-ret closed 4 months ago

johnW-ret commented 6 months ago

Description

Charts passed a Config with Responsive = true still do not scale responsively.

Repro steps

Observe behavior difference when resizing the window on Plotly.NET documentation vs plotly documentation.

Related information

Polyglot Notebooks in VS Code with Plotly.NET, 4.2.0; Plotly.NET.Interactive, 4.2.1.

johnW-ret commented 6 months ago

Comparing the source code from both pages and seems like the appropriate config is getting passed? I don't see anything related to responsiveness in any of the subsequent release notes but maybe could benefit from a version bump to latest plotly anyway?

johnW-ret commented 6 months ago

It appears that the problem is when height or width are manually set in Layout responsiveness breaks.

I get the desired behavior when passing UseDefaults = false to Chart methods. For Plotly.NET docs, it appears Responsive must also be manually set. For my use case (nbconvert), it appears that Responsive does not need to manually be set but AutoSize does? I admit I'm not good at CSS 🙂.

Perhaps Height and Width should be set to float? or float option to represent when they are not passed at all?

kMutagene commented 6 months ago

Hey @johnW-ret

It appears that the problem is when height or width are manually set in Layout responsiveness breaks.

I think that assertion is correct, and would also make sense to me from the plotly.js side.

I have to admit that i am not sure how to 'fix' this, as charts look awful by default in html output if we do not include preset dimensions. if you want to keep the default styles, you could manually remove the width and height settings on the layout.

johnW-ret commented 6 months ago

@kMutagene Thanks for the reply.

My main issue is that UseDefaults = false can't be partially applied because Chart.Foos take tupled parameters, so I can't create a function that only binds UseDefaults on a foo -> bar -> ... -> GenericChart. I have to (?) create a method for each chart type.

I don't necessarily agree about charts looking awful by default, and think that reflecting the source API should be pretty important, but I see what you mean and respect that reasoning.

If DefaultWidth and DefaultHeight were made optional, some control flow code could be added here that conditionally omits them in Layout.init.

let defaultLayout = 
   Layout.init (
      Width = Defaults.DefaultWidth,
      Height = Defaults.DefaultHeight,
      Template = (defaultTemplate :> DynamicObj)
)

I see how this could open pandora's box for a not-nice way of conditionally 'unsetting' all different configurations of default parameters, but the alternative from the consumer side is not very nice.

kMutagene commented 5 months ago

@johnW-ret have you tried using a custom template instead of the default template? It sounds to me like this would solve your issue

johnW-ret commented 5 months ago

@kMutagene

I've played around with the API and looked in source and apologies If I still misunderstand it.

It appears that the only way to ensure Height or Width are unset, which is necessary for Responsive = true charts to actually be responsive, is to pass UseDefaults = false, which is a tupled parameter.

In fact, if you use a Template such as Template.init(Layout.init(Width = 1500)) (either by setting Defaults.DefaultTemplate or using Chart.withTemplate),Width is just ignored and DefaultWidth is always used. This to me feels like a bug.

I'm not really sure what the best solution is. I'm interested to hear what the rationale for DefaultWidth and DefaultHeight were instead of just making them part of the default template. If they were removed and made part of the Template system, you could pass a Template with Layout with Height or Width unset. Alternatively, Height or Width could be made nullable or optional, which feels like a bigger change.

johnW-ret commented 5 months ago

I feel a little dumb. I can just

Defaults.DefaultWidth <- 0

and responsiveness works again.

Any chance a PR I submit to fix responsiveness for the documentation for Responsive charts gets approved?

kMutagene commented 5 months ago

Sure, in all cases that would be better than the broken example that is there right now. however, i feel like this should not be the intended way to make this work.

It appears that the only way to ensure Height or Width are unset, which is necessary for Responsive = true charts to actually be responsive, is to pass UseDefaults = false, which is a tupled parameter.

I do not really get why it being a tupled parameter is an issue, since all parameters in the high level Chart API are tupled parameters.

You have 2 possibilities that are IMO better than setting the default width global value to 0. Also apologies if my previous responses were to brief to be helpful.

  1. Do not use default values (by setting UseDefaults = false), and then setting a custom template after the chart is generated, e.g.

    open Plotly.NET
    
    Chart.Point(xy=[1,2], UseDefaults = false)
    |> Chart.withTemplate(ChartTemplates.plotly)
    |> Chart.withConfigStyle(Responsive = true)
  2. unset the values on the generated Layout via GenericChart.mapLayout:

    Chart.Point(xy=[1,2])
    |> GenericChart.mapLayout(fun l ->
      l.Remove("width") |> ignore
      l.Remove("height") |> ignore
      l 
    )
    |> Chart.withConfigStyle(Responsive = true)

I think these are better solutions, since they do not change global default values that will be used for other function calls. What do you think?

kMutagene commented 4 months ago

Closing this for now. If you want to re-visit this, feel free to re-open @johnW-ret