Conversation
Co-authored-by: John Casagranda <[email protected]> Co-authored-by: Gauthier Segay <[email protected]>
Co-authored-by: John Casagranda <[email protected]> Co-authored-by: Gauthier Segay <[email protected]>
Co-authored-by: John Casagranda <[email protected]> Co-authored-by: Gauthier Segay <[email protected]>
Co-authored-by: John Casagranda <[email protected]> Co-authored-by: Gauthier Segay <[email protected]>
Co-authored-by: John Casagranda <[email protected]> Co-authored-by: Gauthier Segay <[email protected]>
|
@kMutagene if you have an idea how to adjust so that the legend at the top right doesn't overlay on top of the "100%" label, please let me know. I'm wondering also if we should have optional parameter that would take the separate plots to alter them: type ParetoChartPart =
| ColumnPart
| ParetoLinePart
Chart.Pareto(...
, customizeChartPart=[
ColumnPart, (fun chart -> chart |> Chart.with...)
]
)This technique could be used in the different composed charts. |
kMutagene
left a comment
There was a problem hiding this comment.
Hi guys, this looks great. This is in general ready to merge. if you want to improve a bit you can use a single line chart instead of one point and one line chart, and expose more customizability via optional arguments. Otherwise, LGTM!
src/Plotly.NET/ChartAPI/Chart2D.fs
Outdated
|
|
||
| let bars = Chart.Column(Seq.zip orderedLabels orderedValues,?Name=Name) | ||
|
|
||
| let points = |
There was a problem hiding this comment.
there is no need for 2 traces for points and lines here, you can show markers on a line chart with ShowMarkers = true.
| |> Chart.withYAxisStyle ( | ||
| ?TitleText = Label | ||
| , Id = StyleParam.SubPlotId.YAxis 1 | ||
| , ShowGrid = false |
There was a problem hiding this comment.
I usually expose settings like this as method argument, and setting sensible default values.
|
Also, a version using labels and values as separate sequences instead of tupled values would be nice as well, but not necessary. This would have the advantage that we do not need a custom tuple type for the C# version, and instead expose the method using separate sequences (this is what i did with all other C# methods) |
|
@kMutagene thanks for the feedback, we'll go over the suggestions / adjustments probably over Sunday. |
…hart instead, adding overloads to take the labels and values separately, adding ShowGrid as an option. Co-authored-by: John Casagranda <[email protected]> Co-authored-by: Gauthier Segay <[email protected]>
|
@kMutagene I made adjustments, the main remarks I have:
Let see if people need specific customizations. |
Can you give an example of mismatched data? I think i do not understand exactly.
Can also be prevented by styling the markers of the line chart accordingly, but i think this can be done after merging this PR. In general, this is a really high quality PR as it even includes documentation, thanks @smoothdeveloper and @rockfaith75 ! The only thing needed for release is addition of some tests, but since it is quite complicated to set up tests in this repo I will take care of that. Thanks again! |
like let values = [1.;2.;3.]
let labels = ["a";"b"] // missing one label
Chart2D.Pareto(labels, values)Thanks for the review and we'll be on the lookout for how the tests are added for future contributions. |
I see, but this is handled by plotly.js by simply only displaying data that has both dimensions, so we overall do not really care about this across the lib. |
@kMutagene, this is for #423, please let us know what changes you want.
Here is snippet of rendered documentation:
cc: @rockfaith75