-
Notifications
You must be signed in to change notification settings - Fork 409
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
#8755 Multivariable charts #9655
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here some of my findings.
You can start fixing the bugs or define issues to solve later
-
Pie charts on different axis looks to be stacked, they should be grouped. The stacking option do not influences it (Because is part of the trace, stacking correctly only the elements of the same trace).
**In general looks like layout section should splitted in:
-
Layout tab, common for all charts, should be an external tab, as well as "Axes" (depending on the chart type anyway for bar mode etc...). Future we can organize it adding stackgroups, offsetgroups etc...
-
An Axes section of the trace, refering the axes for the current trace.
-
when you open a new trace, the preview returns an error. Usually it shows a sample chart. The message is quite good but the error icon is scaring. I suggest to avoid to show the track until is misconfigured show a less warning icon. Is hard to understand that you have to delete or complete the incomplete trace to have the chart working again.
new_trace_errro.webm
-
When I have strings on y axis, I have a nan in labels on bar chart
-
The button for clearing style customization should have a tooltip, is not clear it's role (maybe also because it is a trash that sounds like delete, not clear. a brush icon is more intuitive, but the tooltip should be there anyway)
screencast-localhost_8081-2023.10.24-19_05_04.webm
-
Also these buttons should have tooltips like: "Edit the chart title", "Edit the trace title", "Add a new trace", "delete the selected trace", "Select a different layer" (layer icon, maybe, or layer add), "filter the layer" (layer filter icon may be a good option too, even if a little redundant, in a so complex UI it helps)
-
"Tab title of "Axis" should be "Axes" (it contains x, y so it is plural).
-
"Trace data" or "data" instead of "Configure data", not uniform with the rest of the labels.( i should remove "Configure "). See below the improvemnt suggested.
Improvements
This section contains improvements that for sure can be done later, or that need a discussion.
- I should remove
[ Trace 1 ]
[Trace 2]
it takes space for no reason. I show it (grayed out) only if the title is not inserted. - hard to understand that the trace options below are related to the trace, maybe because of the sticky scrolling.
Maybe using labels like: - Trace data ( i should remove "Configure ")
- Trace style
- Trace axes (new option, instead of adding to layout) Layout is in common with all the traces, so is should stay in another tab, isn't it?
- Value format
Another option is to have a layout like this, that is more descriptive.
Can help to have a better understanding?
- A little misalignment between combo and buttons, that should be part of the combo somehow ( hard to understand note in the picture refers to tooltips).
- The same for axis settings, is a little hard to understand that where the options are dedicated for one axis and what are global. Maybe "Y axis vallue formatting" look to be global.
- For the documentation of the json format, I should move the things in another issue from now.
- the first new axis should be on the opposite side of the first one, to avoid the user to manually configure it every time. Even right by default on add can be ok.
- The tooltip of the button for "go next step" on top tells me that one or more charts are invalid, but I don't have information about the reason or what charts/traces are invalid, maybe instead of indicating [Trace N] an icon on the right may show in the trace selector the trace that is invalid. This can be an improvement
screencast-localhost_8081-2023.10.25-10_01_09.webm - I tried to create a real life example with 2 axis. Km2 and people. The result is in the video. bbasically the things that do not work are stacking is randomic, thing that I already notified. I think this can be a good real world example to test this functionality, so I attach my result json that I produced (maybe we will have to adjust stacking). Ideally we should be able to:
- stack male/female while global population stays aside (grouped)
- stack land and water km
- Being able to add some other items to population.
screencast-localhost_8081-2023.10.25-10_01_09.webm
- It could be useful to be able to sort traces in the future to avoid the needing to recreate traces to properly sort them up
Attachments
- Configs of stacked chart bugs stacked.zip
- config with NaN error NaN.zip
- population (total, male, female) / surface (land km, water km) chart.
male_km2.zip
@offtherailz Thank you so much. I had a call with @allyoucanmap to review what reported above together. Some points will remain out for the moment and new issues will be opened for future enhancements. Anyway, the most relevant/important ones (the most part in practice) will be worked by @allyoucanmap that will provide a new update by tomorrow morning with a comment indicating what done point by point above. |
@offtherailz @tdipisa I applied the requested changes but the following:
For the moment I kept it in the trace tab changing the name of other section as requested. We can discuss about this new tab to include additional options for the future
The NaN issue was not introduced by this work. The reason is because we are always assuming that the aggregation property is always a number and always a y axis. The hover template of the axis add a default formatting assuming the y is a number (eg see here)
We need to think on how integrate this inside the doc, as I mentioned in the PR description I tried some plugin but they are not working well
I agree that the sorting option could be helpful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When selecting classification style I have a crash
screencast-localhost_8081-2023.10.26-17_41_29.webm
Error in console:
react-dom.development.js:501 Uncaught TypeError: Cannot read properties of undefined (reading 'method')
at Object.onChange (ChartClassification.jsx:334:129)
at Select.setValue (react-select.es.js:1290:16)
at Select.clearValue (react-select.es.js:1379:9)
at HTMLUnknownElement.callCallback (react-dom.development.js:362:14)
at Object.invokeGuardedCallbackDev (react-dom.development.js:411:16)
at invokeGuardedCallback (react-dom.development.js:466:31)
at invokeGuardedCallbackAndCatchFirstError (react-dom.development.js:480:25)
at executeDispatch (react-dom.development.js:612:3)
at executeDispatchesInOrder (react-dom.development.js:634:7)
at executeDispatchesAndRelease (react-dom.development.js:743:5)
at executeDispatchesAndReleaseTopLevel (react-dom.development.js:752:10)
at forEachAccumulated (react-dom.development.js:724:8)
at runEventsInBatch (react-dom.development.js:769:3)
at runExtractedPluginEventsInBatch (react-dom.development.js:914:3)
at handleTopLevel (react-dom.development.js:5848:5)
at batchedEventUpdates$1 (react-dom.development.js:24343:12)
- On the video above you can notice that classification attribute has an x but it can not be cleared. Remove the
x
. - On the video above you can see the custom classification table is misaligned when the value is a number
- Creating multiple pie chart, the 4th for some reason is not visible.
- Editing the title of the trace do not work until the chart is fully set up. Instead it should be edited.
Additional notes
- the first impression of the new UI for notifiyng missing attribute works well but it looks to "WARNING" style 😄 . We should do a better design to indicate mandatory values and when compiled and made invalid again, then notify the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@offtherailz please create tasklist for further identified improvements and assign it to me please. |
@allyoucanmap @tdipisa Here are some notes after my tests. I use this dash
1.mp4
2.mp4
3.mp4
4.mp4
5.mp4
6_dev.mp4
7.mp4 |
@allyoucanmap points 1, 3 and 7 now work |
Description
This PR introduces the concept of multi traces in chart widgets. Changes included in the PR:
Some examples
multiple lines and axis configuration
line-chart.mp4
multiple bars and style configuration
bar-chart.mp4
multiple pie charts and dynamic classification
pie-chart.mp4
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
What is the current behavior?
#8755
What is the new behavior?
Add support for multiple traces in charts
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information
Additional enhancements