Custom legend #637
Custom legend #637
Conversation
|
changed mui to HTML for adding flexibility: only allows string so it is not possible to add markers etc. Also changed some layouts. Will move customLegend to web-components's plotControls and change its name to PlotLegend |
|
moved CustomLegend (now PlotLegend) to web-components's plotControls. Changed its function name to PlotLegend as well |
|
Scatter plot viz has various exceptional cases, like having plot options, thus for handling more general case at first, current custom legend applied to histogram viz. Though, it also has at least one exception that the order of legend items are reversed due to stacked plot: this will be addressed later after dealing with other to-dos first, e.g., filters, No data, markers, etc. |
| // extract legendItems.label for passing to both PlotlyPlot and PlotLegend | ||
| setCheckedLegendItems(legendItems.map((item) => item.label)); | ||
| } | ||
| }, [data]); |
There was a problem hiding this comment.
maybe add legendItems just to be safe?
| filters != null && overlayVariableFilter | ||
| ? filters | ||
| .map((filter) => { | ||
| if (filter.type == 'stringSet' && data.name != null) |
There was a problem hiding this comment.
As discussed on Slack, this needs to look inside the data to see if any bin.count is non-zero
There was a problem hiding this comment.
yes I am changing it by checking data.bins.length. Perhaps bin.count is another way too 👍
| if (data != null && data.series.length > 1) { | ||
| // const nameArray = data.value?.dataSetProcess.series.map((data: any) => data.name); | ||
| // use this to set all checked | ||
| await setCheckedLegendItems(legendItems.map((item) => item.label)); |
There was a problem hiding this comment.
my IDE says the await is not needed, and I think it's correct!
There was a problem hiding this comment.
I also noticed it but without that await, somehow it did not work correctly so I left it as is
There was a problem hiding this comment.
Can you explain what doesn't work, when you remove await?
| if (data != null && data.series.length > 1) { | ||
| // const nameArray = data.value?.dataSetProcess.series.map((data: any) => data.name); | ||
| // use this to set all checked | ||
| await setCheckedLegendItems(legendItems.map((item) => item.label)); |
| filters != null && overlayVariableFilter | ||
| ? filters | ||
| .map((filter) => { | ||
| if (filter.type == 'stringSet' && data.name != null) |
| if (data != null && data.series.length > 1) { | ||
| // const nameArray = data.value?.dataSetProcess.series.map((data: any) => data.name); | ||
| // use this to set all checked | ||
| await setCheckedLegendItems(legendItems.map((item) => item.label)); |
There was a problem hiding this comment.
Can you explain what doesn't work, when you remove await?
|
@dmfalke so, I noticed an thumbnail capture issue at Scatter plot case with the custom legend: from my limited test it does not occur at other viz though. It happens when using plot option other than 'raw', i.e., smoothed mean or best fit, and thumbnail is not made correctly in those cases - showing previous plot. I think that it is because the custom legend's status is asynchronously happened as compared to thumbnail capture you made. So, I made async/await at the same useEffect so that custom legend is firstly updated and then thumbnail capture is done after that. |
|
I think now both histogram and barplot vizs are working correctly with the custom legend. Surprisingly the checkbox behavior (turning on/off plot trace) worked just fine for the faceted plot without much changes (at least for histogram and barplot vizs) 🎉 Since they both used the same marker style (solid square without border), the current custom legend (PlotLegend component) are working for them even if currently it only has that square marker case. Will need to check marker style for other plot vizs and add markers accordingly. |
|
Thanks for the update @moontrip. One suggestion is to store the checkedLegendItems as a part of the visualization config object. If you do this, be sure to make it optional. |
@dmfalke Actually I thought of the case as well because, if I remember correctly, there was a talk to keep the status of legend selection. Perhaps I will think of that case next week as I am off from VEuPathDB today. |
The I think the problem can be solved by moving the "set all checked" useEffect to the main Then the toImage useEffect just needs Here's the diff from what I was playing around with diff --git a/src/lib/core/components/visualizations/implementations/ScatterplotVisualization.tsx b/src/lib/core/components/visualizations/implementations/ScatterplotVisualization.tsx
index fa86efd4..30577b88 100755
--- a/src/lib/core/components/visualizations/implementations/ScatterplotVisualization.tsx
+++ b/src/lib/core/components/visualizations/implementations/ScatterplotVisualization.tsx
@@ -365,6 +365,7 @@ function ScatterplotViz(props: VisualizationProps) {
: [];
}, [data, filters]);
+
//DKDK
console.log(
'data.value?.dataSetProcess.series =',
@@ -377,6 +378,11 @@ function ScatterplotViz(props: VisualizationProps) {
console.log('checkedLegendItems =', checkedLegendItems);
+ useEffect(() => {
+ // use this to set all checked
+ setCheckedLegendItems(legendItems.map((item) => item.label));
+ }, [legendItems]);
+
return (
<div style={{ display: 'flex', flexDirection: 'column' }}>
<div style={{ display: 'flex', alignItems: 'center', zIndex: 1 }}>
@@ -499,7 +505,6 @@ function ScatterplotViz(props: VisualizationProps) {
//DKDK pass checked state of legend checkbox to PlotlyPlot
checkedLegendItems={checkedLegendItems}
setCheckedLegendItems={setCheckedLegendItems}
- legendItems={legendItems}
/>
{/* DKDK custom legend */}
@@ -573,8 +578,8 @@ type ScatterplotWithControlsProps = XYPlotProps & {
// add disabledList
disabledList: string[];
//DKDK legend checkbox
+ checkedLegendItems: string[];
setCheckedLegendItems: (checkedItems: string[]) => void;
- legendItems: LegendItemsProps[];
};
function ScatterplotWithControls({
@@ -589,8 +594,8 @@ function ScatterplotWithControls({
disabledList,
updateThumbnail,
//DKDK legend checkbox
+ checkedLegendItems,
setCheckedLegendItems,
- legendItems,
...scatterplotProps
}: ScatterplotWithControlsProps) {
// TODO Use UIState
@@ -613,20 +618,17 @@ function ScatterplotWithControls({
//DKDK add async/await for correct thumbnail capture
useEffect(() => {
(async () => {
- if (data != null) {
- // use this to set all checked
- await setCheckedLegendItems(legendItems.map((item) => item.label));
- }
await plotRef.current
?.toImage({ format: 'svg', ...plotDimensions })
.then(updateThumbnailRef.current);
})();
- }, [data, legendItems]);
+ }, [data, checkedLegendItems]);
return (
<div style={{ display: 'flex', flexDirection: 'column' }}>
<XYPlot
{...scatterplotProps}
+ checkedLegendItems={checkedLegendItems}
ref={plotRef}
data={data}
// add controls |
|
However, a bigger structural change is that we need the checked items (if there are any) to be saved in (which I see Dave already mentioned) |
@bobular thank you for your suggestion. Actually the approach I have used async/await for both legendItems and thumbnail at useEffect was designed for scatter plot and it worked just fine. I don't know why you did not have the screenshot correctly. Anyway, the approach you suggested was my initial approach to use useEffect for legendItems (set function) at ScatterplotViz but it didn't work properly for me, especially with plot options with smoothed mean and best fit line: that was the reason why I switched to async/await. However, what I missed is the dependency at useEffect of thumbnail so it may make difference. I will test it too! And yes I started to set vizConfig.checkedLegendItems for histogram viz and then barplot viz. If it would be done, I think that we can merge this custom legend stuff so that I can add this custom legend to other plot vizs that are being grounded by you 😄 |
|
@bobular after testing, the missing dependency at thumbnail's useEffect() at my implementation was the culprit to prevent from correct capture 👍 I should not have thought of more complex solutions rather than the simple one 😄 |
|
Great! Good catch @bobular, and nice work @moontrip! @moontrip, there is a long-standing issue to retain the state of what legend items are visible. |
@dmfalke I will think of that specific case later: need to make custom legend work for all vizs, including faceted plot, at first :) |
|
@bobular and @dmfalke histogram and barplot vizs with faceted plot are now changed to use vizConfig. @bobular Actually I also changed scatterplot viz in this branch (i.e., without faceted plot) to use vizConfig, but I am hesitating to commit because you already started working previous scatter plot to have faceted plot: in that case I can change yours later. If you didn't start the scatter plot yet, then I will also commit it here. Just let me know 👍 |
Hi @moontrip - I haven't started on scatterplot faceting - so please commit! |
Thank you for your info, @bobular 👍 I just committed scatter plot viz as well. |
|
now make this as a normal PR so that it can be used for other Vizs with faceted plot, which I will handle the addition of this custom legend to those vizs - done for histogram and barplot vizs, and scatter plot viz in part. |
src/lib/core/components/visualizations/implementations/HistogramVisualization.tsx
Show resolved
Hide resolved
|
Reviewing as fast as I can now. From testing, I see there's a general logic/wiring issue that I'll make another ticket for and fix at a later date. Here's the ticket: #676 We shouldn't show the gray No data option for overlay variable sex in the example below. The problem is that |
|
Thanks @bobular ! 👍 I also saw your report regarding the bug(?). Initially I thought I made a mistake to incorrectly show the legend item, No data, but after reading the description carefully, I realized that it was related to the switch behavior. Glad that my custom legend helps to identify the issue (and also relieved myself a bit that the legend logic worked fine :) ) |
| @@ -8,7 +8,7 @@ export function useFindOutputEntity( | |||
| // need to add string at Record's Type due to valueSpecConfig | |||
| dataElementVariables: Record< | |||
| string, | |||
| VariableDescriptor | string | boolean | undefined | |||
| VariableDescriptor | string | boolean | string[] | undefined | |||
There was a problem hiding this comment.
Oh, I see why... because we pass the whole of vizConfig to this function. Looks like some technical debt here!
| }, [data]); | ||
|
|
||
| // use this to set all checked | ||
| useEffect(() => { |
There was a problem hiding this comment.
I thought I had a simple solution to the problem of this overwriting the saved configuration, but it wasn't so simple.
So are we going to solve this separately? Don't forget to make a ticket if so.
There was a problem hiding this comment.
will make a ticket to handle saving/restoring checked legend items. Actually legend markers are for both boxplot and scatter plot as boxplot utilized different marker style unlike histogram etc.: square but light color and border for boxplot ones.
There was a problem hiding this comment.
As pointed out by Dave, we already have saving/restoring ticket made long time ago, #172, so I don't need to make it: . just assigned it to me so that I won't forget 😃 Considering other tasks I have to do for b55, I will defer it, though.
|
thank you for your review and comments @bobular ! 👍 Will work for merging |
@bobular @dmfalke I have made PR for initial custom legend works presented at the dev call. This relies on the PR at web-components as well.