Add legend to zoomed in plots #1794
Conversation
mattseddon
force-pushed
the
add-legend-to-zoomed
branch
from
ed8d770 to
170100d
Compare
| $schema: 'https://vega.github.io/schema/vega-lite/v5.json', | ||
| data: { name: 'values' }, | ||
| encoding: { | ||
| color: { |
There was a problem hiding this comment.
[F] The change is to move the color out here so that we can overwrite it in the same way we do for Data Series plots.
mattseddon
requested review from
sroy3,
rogermparent,
shcheklein and
wolmir
and removed request for
wolmir,
shcheklein,
sroy3 and
rogermparent
There was a problem hiding this comment.
I know this isn't ready for review, but wanted to see if I could help on your bug
checkpoint plots will not render with their legend if they are the first plot that has been zoomed in the plots view
I pulled your branch and wasn't able to reproduce. Though removing all plots and adding them again, the legend is sometimes in all the plots:
Screen.Recording.2022-05-31.at.11.01.04.AM.mov
I think that this can be solved by applying the change I mentioned about removeLegendSuppression.
| const handleZoomInPlot = useCallback( | ||
| (props: VegaLiteProps, id: string, refresh?: boolean) => { | ||
| if (!refresh) { | ||
| setZoomedInPlotWithLegend(props) | ||
| setZoomedInPlot(removeLegendSuppression(props)) |
There was a problem hiding this comment.
Instead of having removeLegendSuppression everywhere, can we just add its content to the VegaLite component on line 159?
There was a problem hiding this comment.
This is definitely the better way to go. I still have a couple of issues hanging around though:
- The Storybook is completely cooked. Legend's randomly turn up in plots on random stories.
useEffectin<ZoomablePlot/>causes an infinite loop if you don't exclude disabling of the legend from the props comparison at the top of the function.
Will investigate more today with a fresher(-ish) mind. Thanks a lot for looking!
|
Code Climate has analyzed commit 0f7848f and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.8% (0.0% change). View more on Code Climate. |
Closes #1782 by adding the relevant legend to zoomed-in plots (both Data Series & Trends).
Demo
Screen.Recording.2022-06-01.at.10.02.10.am.mov
Screen.Recording.2022-06-01.at.10.01.41.am.mov
Screen.Recording.2022-06-01.at.10.08.32.am.mov
Thanks to @sroy3 for assisting my mash potato brain with fixing some bugs in this PR!