Fix/Feat: Maintain consistency of assigned comparison colors #4198
Conversation
briangregoryholmes
changed the title
|
@briangregoryholmes : Is this still a draft ? wondering if we can get this over the finish line for current release ? |
briangregoryholmes
changed the title
|
@nishantmonu51 Took care of the last outstanding issue. Ready for review. |
There was a problem hiding this comment.
Have added some comments around usage of $page and other changes.
Apart from that seeing some regression. When max colored comparison is reached, toggling off a dimension value doesn't change the colors of inactive dimensions.
Kapture.2024-03-18.at.18.23.36.mp4
Apart from that, for Leaderboard and Dimension Detail table, the max limit is 7 and for TDD it's 11 (since it has a bigger chart surface to accommodate more lines)
web-common/src/features/dashboards/dimension-table/DimensionFilterGutter.svelte
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/leaderboard/LeaderboardListItem.svelte
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/leaderboard/LeaderboardListItem.svelte
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/time-dimension-details/TDDTable.svelte
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/time-dimension-details/TimeDimensionDisplay.svelte
Outdated
Show resolved
Hide resolved
| $: colors = dimensionData.map((d) => { | ||
| return colorGetter.get($page.params.name, d.dimension, d.value ?? ""); | ||
| }); |
There was a problem hiding this comment.
Since this is only used for MeasureChart, we can move this inside the component.
There was a problem hiding this comment.
It's placed here because the colors are not unique to each MeasureChart, but rather to the MetricsTimeSeriesChart view. They only need to be fetched once regardless of the number of charts. Also, because of some upstream issues with TimeSeriesChartContainer and GraphicContext, MeasureChart will re-render dozens of times when you scrub or add new filter pills so placing it there would add a lot of unnecessary calls.
web-common/src/features/dashboards/time-series/ChartBody.svelte
Outdated
Show resolved
Hide resolved
|
Updated to use metricsViewName throughout. Your point about the regression is a fair one, though I'll rope in @mindspank @jkhwu for their thoughts since I think there are multiple solutions. In my view, we should never be reassigning a comparison color without explicit user interaction, even inactive ones. If I deselect blue, I expect the chart to no longer be showing a blue line. As such, I updated the PR to match the "capping" behavior in a way that still maintains that goal. Clicking an eighth comparison assigns it, for instance, |
|
@briangregoryholmes : bumping this up. Its open from a long time. Lets try to get this over the finish line this week. +cc @ericpgreen2 |
This PR changes the logic by which comparison colors are assigned. Previously, if you had three values selected, they would be assigned blue, orange and green in order. If you then toggled the blue value, the other colors would be re-assigned. Orange would turn blue and green would turn orange. With this PR, toggling the blue value simply frees blue up to be assigned to the next selected value and does not change any of the other colors.
This is a temporary solution to a problem that will eventually be solved by a state management refactor. There are also some quirks in the way this is implemented that have to do with how the
TDDTablecomponent works, which will also be refactored soon.