Persisting colors across chart type changes and field changes #2132
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Hey @noahonyejese, thanks for implementing this 🙇♂️
It goes into a good direction, but there are some tweaks need before we go further (e.g. we have an application crash when switching to a combo chart). Let me know if some of my comments are not clear 👍
| const maybeSegmentField = convertTableFieldsToSegmentFields({ | ||
| fields: oldValue as TableFields, | ||
| }); |
There was a problem hiding this comment.
@noahonyejese I am not exactly sure where the code is wrong, but you can see that the colors are set wrongly when going to Photovoltaikanlagen cube, adding segmentation by Kanton, switching to a table chart and back to a column chart – we get all blue bars then 👀
There was a problem hiding this comment.
@noahonyejese I checked the same flow as above, and now it looks like it's the same (and we don't show a legend now) :(
app/charts/index.ts
Outdated
| color: ({ oldValue, newChartConfig }) => { | ||
| return produce(newChartConfig, (draft) => { | ||
| draft.fields.color = oldValue as BarFields["color"]; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
@noahonyejese another thing I've noticed is when you go to a column chart in Photovoltaikanlagen cube, switch X axis to Kanton, add segmentation by year, switch to a line chart and then back to a column chart – the color is black then 🤔
There was a problem hiding this comment.
No longer at least not sure why it was happening before, or maybe I didn't replicate this properly 😅
There was a problem hiding this comment.
@noahonyejese this is also still the same unfortunately :(
… & tables & bars single color
this is not yet working
There was a problem hiding this comment.
@noahonyejese it looks like the issues we had before are still there, could you take a look at them? 🙏
I think we also now have a new one – when switching from a pie chart to a column line chart in Photovoltaikanlagen, the application breaks (I think it might be related to migrating measures field without adjusting in pie chart).
I think here it would be good to "click through" different chart types with and without segmentation and see how they behave, I've also noticed that sometimes the segment colors are all blue 🟦
| switch (oldValue.type) { | ||
| case "measures": | ||
| draft.fields.color = { | ||
| ...oldValue, | ||
| type: "segment", | ||
| }; | ||
| break; |
There was a problem hiding this comment.
@noahonyejese I think this logic needs to be adjusted – if we switch from a combo chart to a pie chart, we can't take the old color mapping over, as it was based on measure ids, not on dimension values as needed in regular charts' segmentations. Due to this, now when we switch from a combo chart to a pie chart, we get all blue slices:
Same for scatterplot 👍
| switch (oldValue.type) { | ||
| case "measures": | ||
| draft.fields.color = oldValue; | ||
| break; |
There was a problem hiding this comment.
@noahonyejese looks like we have some duplicated logic, as the color field is being assigned both here and also in y -> componentIds adjuster. I think we should have only one place to do it – or did I miss something? 🤔 It's the same for other combo charts 👀
| const maybeSegmentField = convertTableFieldsToSegmentFields({ | ||
| fields: oldValue as TableFields, | ||
| }); |
There was a problem hiding this comment.
@noahonyejese I checked the same flow as above, and now it looks like it's the same (and we don't show a legend now) :(
app/charts/index.ts
Outdated
| color: ({ oldValue, newChartConfig }) => { | ||
| return produce(newChartConfig, (draft) => { | ||
| draft.fields.color = oldValue as BarFields["color"]; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
@noahonyejese this is also still the same unfortunately :(
noahonyejese
changed the title
Closes #2097
What changes?
This PR allows colors to persist over chart type changes.
How to test
How it's currently