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

Chart builders with data parameter for all charttypes #1132

Open
HoldYourWaffle opened this issue Dec 9, 2019 · 5 comments · May be fixed by #1145
Open

Chart builders with data parameter for all charttypes #1132

HoldYourWaffle opened this issue Dec 9, 2019 · 5 comments · May be fixed by #1145

Comments

@HoldYourWaffle
Copy link
Contributor

Is there a reason why only the piechart builder has a data parameter?

/**
* Create a PieChart with optional title data and add to the parent pane. The optional op will be performed on the new instance.
*/
fun EventTarget.piechart(title: String? = null, data: ObservableList<PieChart.Data>? = null, op: PieChart.() -> Unit = {}): PieChart {
val chart = if (data != null) PieChart(data) else PieChart()
chart.title = title
return opcr(this, chart, op)
}

/**
* Create a LineChart with optional title, axis and add to the parent pane. The optional op will be performed on the new instance.
*/
fun <X, Y> EventTarget.linechart(title: String? = null, x: Axis<X>, y: Axis<Y>, op: LineChart<X, Y>.() -> Unit = {}) =
LineChart<X, Y>(x, y).attachTo(this, op) { it.title = title }

@edvin
Copy link
Owner

edvin commented Dec 10, 2019

I believe that's because PieChart only has a single dimension, whereas the other charts support multiple dimensions so it's more convenient to generate series of data using the series builder after creating the chart.

@HoldYourWaffle
Copy link
Contributor Author

HoldYourWaffle commented Jan 1, 2020

That makes sense. However, in my application the Series are generated programmatically too, so I'm directly binding data to an ObservableList<XYChart.Serie> like this (pseudo-code):

val series = observableListOf<XYChart.Serie>();

linechart {
    data.bind(series);
}

Would it make sense to add a convenience constructor parameter for this use-case?

@edvin
Copy link
Owner

edvin commented Jan 2, 2020

Sure :) It's in line with how the other builders work, so a PR would be welcome :)

HoldYourWaffle added a commit to HoldYourWaffle2/tornadofx that referenced this issue Jan 2, 2020
@ctadlock
Copy link
Collaborator

@HoldYourWaffle I’ll get this merged in. Can you also submit one for the jdk10 branch to make it easy? ;)

@HoldYourWaffle
Copy link
Contributor Author

@ctadlock Here you go

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

Successfully merging a pull request may close this issue.

3 participants