Conversation
Added comparison but still don't see access to stacked bar or line
|
You need to add dimension comparison for it to be active. With just time comparison, the stacked area chart is identical to the line chart and the stacked bar chart is identical to the bar chart. |
|
oh right -_- |
I see we're using either old component for chart toggle, or tab component for it? The behavior is not consistent between TDD and leaderboard page. If we had tab component for it, see screenshot 1 for color and state reference. Option 2: If we follow new design use toggle component, then see screenshot below for color and state reference. General rule:
Since Option 1 is already defined in the TDD, we should go with that and ensure consistency in both appearance and behavior across placements. |
|
From Slack conversation: 
Make sure each of the icons have hover tooltips:
|
|
LGTM |
AdityaHegde
left a comment
There was a problem hiding this comment.
Chart type is not saved for public urls. The reason seems to be that we store chart type under tdd. We are adding an explicit to toExploreUrlParams so bookmarks and other url params based checks work. We should perhaps also add something similar to toProto and fromProto.
When in bar chart mode, when time range is equal to grain, i.e. 24hrs and day grain, the scrub selectors breaks. In line chart mode the selection is correctly snapped to grain but not in bar chart more. The selection also doesnt sync to the correct range to other measures.
| * Add this to a layer's `params` array to enable brush/scrub selection. | ||
| */ | ||
| export function createBrushParam(): SelectionParameter { | ||
| const scrubColor = resolveCSSVariable("var(--color-theme-200)"); |
There was a problem hiding this comment.
How about still using the constant ScrubBoxColor within resolveCSSVariable?
| } | ||
| } | ||
|
|
||
| if (searchParams.has(ExploreStateURLParams.ForceLineChart)) { |
There was a problem hiding this comment.
What happens to old urls with line_chart=true
| // Chart display settings (frontend-only; persisted in URL state) | ||
| optional bool chart_dynamic_y_axis = 35; | ||
| optional bool chart_force_line = 36; | ||
| reserved 36; // was chart_force_line; replaced by chart_type URL param |
There was a problem hiding this comment.
Wont we need this for legacy bookmarks?
| expect(getCleanMetricsExploreForAssertion()).toEqual(initState); | ||
| // Chart type is now shared across views, so capture the canonical state | ||
| // after a round-trip (chart type may have changed via the other view). | ||
| const initStateAfterRoundTrip = getCleanMetricsExploreForAssertion(); |
There was a problem hiding this comment.
What is the use case where chart type is changed in a different view? The idea of this assertion is to make sure params dont change when switching views without user interactions.
Add chart type selector similar to TDD in explore view
https://linear.app/rilldata/issue/APP-797/provide-a-way-for-users-to-change-chart-type-in-metric-charts-in
Checklist: