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

ui-chart - change line thickness #1372

Open
Paul-Reed opened this issue Oct 9, 2024 · 19 comments
Open

ui-chart - change line thickness #1372

Paul-Reed opened this issue Oct 9, 2024 · 19 comments
Labels
feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do

Comments

@Paul-Reed
Copy link
Contributor

Paul-Reed commented Oct 9, 2024

Description

Adding a chart to a dashboard to be viewed on phones only, and it would be great if users could designate a line thickness for the chart.
Currently, the line looks as though it's been drawn with a marker pen, and looks even worse when using more that one line.
Using a template node, it's possible to set the line to 1px, which looks clean & sharp, but of course as discussed here, using a template node brings other problems.
By example, the image below shows the default ui-chart node (top), and a template node chart set to line width 1px (bottom).

Have you provided an initial effort estimate for this issue?

I have not provided an initial effort estimate

@Paul-Reed Paul-Reed added feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do labels Oct 9, 2024
@colinl
Copy link
Contributor

colinl commented Oct 9, 2024

How have you configured the Point style and Radius for the line?
Try None and 1

@Paul-Reed
Copy link
Contributor Author

Paul-Reed commented Oct 9, 2024

They were already set as 'None' & 1, and get the same with 'None' & 0.

config

@bartbutenaers
Copy link
Contributor

bartbutenaers commented Oct 9, 2024

Hi @Paul-Reed,

It looks like ChartJs support a borderWidth property.
As you can see e.g. in this demo, that property can be specified in every dataset:

var lineChartData = {
    labels: [...],
    datasets: [{
      data: [....],
      ...
      borderWidth: 1,
}

That way each line in the chart can have its own thickness.

However I 'think' currently you cannot simply pass that property yourself via the input message to a ui-chart node.
But I can be mistaken because I only had a quick look:

  1. When the chartjs object is created, there will be no datasets:

    const config = {
       type: this.props.chartType,
       data: {
          labels: [],
          datasets: []
       },
       options: { ...}
    }
    
  2. The specified datasets will be appended to that datasets array. Every dataset is being created based on the properties from the node's config screen:

    const series = {
       backgroundColor: colorByIndex ? this.props.colors : this.props.colors[sLabels.length % this.props.colors.length],
       pointStyle: this.props.pointShape === 'false' ? false : this.props.pointShape || 'circle',
       pointRadius: radius,
       pointHoverRadius: radius * 1.25,
       label,
       data
    }
    
    if (!colorByIndex) {
       series.borderColor = this.props.colors[sLabels.length]
    }
    
    this.chart.data.datasets.push(series)
    

    BTW I thought that all nodes had been refactored to support dynamic properties, but that doesn't seem to be the case. For example this.props.colors means the colors are used from the config screen, and not from msg.ui_update (otherwise this.getProperty("colors") would have been used). So that means you also cannot overwrite the line width that way either.

  3. In the remainder of the code, only the data points from these datasets can be updated. I don't see any code to update afterwards other (non-data related) dataset properties.

So it looks to me that the line width should be added to the config screen, and then use that while constructing the new ChartJs object. Don't think there is another way to solve or workaround your problem...

I could try to add it to the config screen, but:

  • That same thickness would be applied to ALL lines in the chart.
  • Not everybody in our community likes to have a large amount of properties on their config screens, so not sure if it is ok to add it.

@colinl
Copy link
Contributor

colinl commented Oct 9, 2024

They were already set as 'None' & 1,

I guess your screen must be lower resolution that mine then, so the pixels are bigger.

@Paul-Reed
Copy link
Contributor Author

Paul-Reed commented Oct 9, 2024

Thanks Bart

BTW I thought that all nodes had been refactored to support dynamic properties, but that doesn't seem to be the case

That's why I still have Flexdash dashboards! as I cannot replicate my existing charts.
I posted a year ago about Flexdash formatting and hoped that a similar solution could be adopted in DB2, as it provided a full suite of options, such as left & right y-axis, fixed colours for data points, solid, dashed, dotted lines.... etc
Maybe my fault as I didn't raise an issue.

A better screenshot example of why I raise this now, see the bottom two charts;

@bartbutenaers
Copy link
Contributor

I can't remember all the discussions anymore, but perhaps the ui-chart node was not refactored yet for dynamic properties (via msg.ui_update) because ChartJs still needs to be replaced by another charting library. Which might be much more work if more ChartJs specific settings are being used in the ui-chart node. Black hole in my brain.

@bartbutenaers
Copy link
Contributor

@joepavitt,
I you should find some time to give your opinion about this one, I might try to create a pull request...

@bartbutenaers
Copy link
Contributor

FYI there was another discussion related to this. Beside the line thickness, there are a number of other properties on series-level (e.g. line color, ...) that need to become configurable. Moreover depending on the chart type (line, scatter,...) other properties would become applicable.

To manage all of this in a simple editableList is unfortunately not possible. So a new widget needs to be created. But that is a lot of work, and the core team members are too busy at the moment.

@joepavitt
Copy link
Collaborator

To manage all of this in a simple editableList is unfortunately not possible. So a new widget needs to be created. But that is a lot of work, and the core team members are too busy at the moment.

Thanks for updating this @bartbutenaers - it's on the list (as are many many other things :D )

@bartbutenaers
Copy link
Contributor

@joepavitt
I see on Discourse that @Paul-Reed is struggling with ui-templates to get the job done. Could it perhaps help him to work once the other way around here: I add support to dynamically override some basic series-level properties via input messages, and ofterwards - whenever a widget becomes avaliable - we make them adjustable via the config screen.

If ok, I assume it should be via ´msg.ui_update.series´?

@Paul-Reed
Copy link
Contributor Author

That would be a good opportunity to test the features, without initially exposing them in the UI or documentation.

@joepavitt
Copy link
Collaborator

whenever a widget becomes available - we make them adjustable via the config screen.

This will be involving a larger overhaul than i think you're realising on the front-end side too. It's not just as simple as having the widget available in the Node-RED Editor.

If ok, I assume it should be via ´msg.ui_update.series´?

I'm not a fan of introducing temporary stop-gaps like that, once we support it, we're glued to that for a long time, and in this case, we're likely to have a property called series covering the new editor in the future.

I could be persuaded by something like msg.ui_update.chartConfig and nested options therein?

@bartbutenaers
Copy link
Contributor

This will be involving a larger overhaul than i think you're realising

Not sure why you think I am underestimating it. I did the proposal this morning for the dynamic properties, just because I am aware that the config screen changes are a lot of work. So I am not expecting it in the near future at all. And I wanted to give people like Paul a workaround via the input messages in short term...

For me msg.ui_update.chartConfig is very self-explaining. If @Paul-Reed could provide me a list of 'basic' series-level properties, then I will try to find some time to implement it.

@Paul-Reed
Copy link
Contributor Author

If @Paul-Reed could provide me a list of 'basic' series-level properties

Thanks Bart
A full list, including default settings is detailed at https://www.chartjs.org/docs/latest/charts/line.html#line-styling but if I were to pick the one's that I think would be most popular, and appear fairly basic...

borderColor 
fill
backgroundColor
borderDash
borderWidth
pointRadius
pointStyle
tension

Maybe not possible now, but worth considering some time in the future would be to allow two y-axis's to be used
Currently we are limited to just one y-axis (and one scale), meaning if you were charting a motor's performance on 2 datasets, motor rpm - scale 0-7000, against motor temperature - scale 0-100, the temperature range would be overwhelmed by the scale 0-7000 and of no practical value.
A chart.js example of a left & right Y-axis with different scales is shown at https://masteringjs.io/tutorials/chartjs/two-y-axes which doesn't appear complicated, but maybe difficult to implement using msg.ui_update.chartConfig to format.

@bartbutenaers
Copy link
Contributor

I had a quick look now at the ui-chart node, but would like to get some thoughts from you guys.
Because implementing dynamic properties before the config screen is refactored, has triggered some questions in my head...

Question 1 - temporarily add an editableList?

Currently the chart and series related properties are a bit mixed in the config screen. I tried to mark the series-level properties with a rectangle:

image

I assume that the chart type (linear, bar, ...) should be configurable at series-level, to allow mixed charts to become possible? But if that is the case, I would have a contradiction between the the config screen (where there is only 1 type) and the input messages (where there is 1 type per series).

So I was wondering if I could do it like this: I add an editableList on the config screen, where I show only the current settings per series (i.e. only the chart type and the line color). All other series-level configs (see list from paul) will only be possible via the input messages. And whenever in the future a new widget would be available, the editableList can be replaced by the new widget. But then at least the very basic configs (type and color) for each series are nicely grouped together in the config screen.

Question 2 - chartConfig

I first thought the msg.ui_update.chartConfig was a good property name, but now I am wondering: isn't everything in the config screen about chart config? For example for me the x-label of the chart is also chart config for me, but that would be put e.g. in msg.ui_update.x_label (and not in msg.ui_update.chart_config.x_label). It is currently not clear to me what the chart is: the entire stuff inclusive the axis, or only the line itself. Wouldn't it be more self-explaining to have something like msg.ui_update.datasetConfig= [], which contains a config object for each dataset.

Thanks for thinking out loud!!

@bartbutenaers
Copy link
Contributor

Hi @Paul-Reed,
There is another related discussion about this. To be honest I don't see the trees through the forest anymore at the moment for ui-chart, especially since I have almost not used the ui-chart node myself. And unfortunately I don't have time at the moment to analyse all discussions and create a proposal. But as soon as anybody can provide me some more detailed instructions on how it should work, I will jump in and try to implement it!

@Paul-Reed
Copy link
Contributor Author

Hi Bart, sorry to hear that you are not getting much support with this, especially as you were volunteering to enhance this node in your own time as a community contributor.

The tone of the responses so far suggest to me that opening up chart options is a low priority, and won't be addressed any time soon.

But thanks for your enthusiasm.

@bartbutenaers
Copy link
Contributor

bartbutenaers commented Oct 27, 2024

Hi @Paul-Reed,
Sorry for the delay!
My intention was to remove some work from the shoulders of Joe and his team. I certainly don't want to achieve the opposite here, by giving them extra stress...

But the ui-chart node is a bit of a difficult one for me to get started, because there are a lot of feature requests are going on. If I could get some pointers how it should look like and behave, I could try to implement some features. Doesn't have to be a big bang because that would be a lot of analysis work for the the team, but step by step adding small features is good enough for me. That would even fit better in my free time...

@TimosCodeHub
Copy link

Just a general thought on what Bart mentioned about not every user wanting more and more configuration settings: since the FlowFuse dashboard’s philosophy is ‘the easy things should be easy, and the hard things should be possible,’ I wonder if a toggle switch for normal/expert mode might be worth considering.

I’ve seen this approach work well in other software I use, both for beginner and advanced users, as it provides a balance of simplicity and flexibility. I’m not sure about the challenges or downsides that might come with implementing this, but it might be an interesting path to explore for db2.0. Just putting it out there! 🤷🏼‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do
Projects
Status: Backlog
Development

No branches or pull requests

5 participants