fix(dashboards): control legends individually #78675
Conversation
github-actions
bot
added
the
Scope: Frontend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #78675 +/- ##
========================================
Coverage 78.27% 78.27%
========================================
Files 7120 7127 +7
Lines 313496 313764 +268
Branches 51171 51221 +50
========================================
+ Hits 245388 245612 +224
- Misses 61679 61717 +38
- Partials 6429 6435 +6 |
Bundle ReportChanges will decrease total bundle size by 93.58kB (-0.3%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
| dashboard: { | ||
| id: 'new', | ||
| title: 'Dashboard', | ||
| createdBy: undefined, | ||
| dateCreated: '2020-01-01T00:00:00.000Z', | ||
| widgets: [widget], | ||
| projects: [], | ||
| filters: {}, | ||
| }, |
There was a problem hiding this comment.
I think dashboard: DashboardFixture() would be fine here. Applies to all the other spec files, too! If you can use the fixtures, it'd be a lot cleaner (I saw you did in one spot)
| const dashboardLegendUtils = new DashboardLegendEncoderDecoder({ | ||
| location: initialData.router.location, | ||
| dashboard: { | ||
| id: 'new', | ||
| title: 'Dashboard', | ||
| createdBy: undefined, | ||
| dateCreated: '2020-01-01T00:00:00.000Z', | ||
| widgets: [mockWidget], | ||
| projects: [], | ||
| filters: {}, | ||
| }, | ||
| organization: initialData.organization, | ||
| router: initialData.router, | ||
| }); |
There was a problem hiding this comment.
Could you save this in a variable at the top of the spec and re-use for all tests?
| const [disabledLegends, setDisabledLegends] = useState<{[key: string]: boolean}>( | ||
| decodeList(location.query[WidgetViewerQueryField.LEGEND]).reduce((acc, legend) => { | ||
| acc[legend] = false; | ||
| return acc; | ||
| }, {}) | ||
| dashboardLegendUtils.getLegendUnselected(widget) |
There was a problem hiding this comment.
Since dashboardLegendUtils keeps all this stuff in the URL now, is it possible to remove disabledLegends and setDisabledLegends?
| router: InjectedRouter; | ||
| }; | ||
|
|
||
| class DashboardLegendEncoderDecoder { |
There was a problem hiding this comment.
Accurate name, but I think WidgetLegendState or WidgetLegendSelectionState would be more evocative! ...State implies that the class is used to manage a state, which is true! Plus, removing "Dashboard" from the name is good, since we'll probably want to use this for non-Dashboard UIs in the future!
Oh also, the file name should match, so widgetLegendState.tsx or whichever you go with
| } | ||
|
|
||
| // Updates legend param when a legend selection has been changed | ||
| updateLegendQueryParam(selected: Record<string, boolean>, widget: Widget) { |
There was a problem hiding this comment.
I think setWidgetSelectionState is clearer here. Maybe even
| updateLegendQueryParam(selected: Record<string, boolean>, widget: Widget) { | |
| setWidgetSelectionState(widget: Widget, selection: LegendSelection) { |
And
type LegendSelection = Record<string, boolean>;Somewhere higher up.
This way the class name nicely matches the kind of data that's passed around. WidgetLegendSelectionState manages widgets and selections
| const legendValues = legend.split('-'); | ||
| const widgetId = legendValues[0]; | ||
| const seriesNames = legendValues.splice(1); |
There was a problem hiding this comment.
| const legendValues = legend.split('-'); | |
| const widgetId = legendValues[0]; | |
| const seriesNames = legendValues.splice(1); | |
| const [widgetId, ...seriesNames] = legend.split('-'); |
If you stick to using dash. Otherwise
const [widgetId, seriesNameString] = legend.split('-');
const seriesNames = seriesNameString.split(',');If you go with commas
| const legendValues = legend.split('-'); | ||
| const widgetId = legendValues[0]; | ||
| const seriesNames = legendValues.splice(1); | ||
| if (widget.id === widgetId && seriesNames) { |
There was a problem hiding this comment.
I think this is fine, but it would be nicer, I think, to use .find and find the right widget by ID right away after decoding.
| seriesNames.forEach(series => { | ||
| if (series) { | ||
| acc[this.encodeSeriesNameForLegend(series, widget.id)] = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
If it's possible for series to be empty, maybe series.map(Boolean).forEach would be a little cleaner
| encodeSeriesNameForLegend(seriesName: string, widgetId?: string) { | ||
| return `${seriesName}:${widgetId}`; | ||
| } | ||
|
|
||
| decodeSeriesNameForLegend(encodedSeriesName: string) { | ||
| return encodedSeriesName.split(':')[0]; | ||
| } | ||
|
|
||
| // change timeseries names to SeriesName:widgetID | ||
| modifyTimeseriesNames(widget: Widget, timeseriesResults?: Series[]) { | ||
| return timeseriesResults | ||
| ? timeseriesResults.map(series => { | ||
| return { | ||
| ...series, | ||
| seriesName: this.encodeSeriesNameForLegend(series.seriesName, widget.id), | ||
| }; | ||
| }) | ||
| : []; | ||
| } |
There was a problem hiding this comment.
IMO this would be even better ins a separate class, or even as a set of helper functions!
| } | ||
|
|
||
| // when a widget has been changed/added/deleted update legend to incorporate that | ||
| updatedLegendQueryOnWidgetChange(newDashboard: DashboardDetails) { |
There was a problem hiding this comment.
🤔 this one is a bit odd. Is it possible to call encodeLegendQueryParam for every new widget instead of this?
Previously charts that have the same series on their legend were controlled together (toggling one off would toggle them both off). Now the legend can be controlled individually as they are associated with the widget id. Since it is stored in the query (in the format
widgetId-seriesName1-seriesName2-...), this also fixes the issue where selecting a new time range or other filter changes reset what series were selected.video evidence:
Screen.Recording.2024-10-07.at.9.32.03.AM.mov