fix: analytics chart kebab menu positioning #1785
Conversation
show kebab menu on hover only if there's no title overlap canvas and show on hover
| @@ -302,7 +302,7 @@ describe('<AnalyticsChart />', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| it('Renders an "Export" button, and tabulated data in the modal preview', () => { | |||
| it.only('Renders an "Export" button, and tabulated data in the modal preview', () => { | |||
| @@ -153,7 +153,6 @@ const dashboardConfig: DashboardConfig = { | |||
| { | |||
| definition: { | |||
| chart: { | |||
| chartTitle: 'Timeseries line chart of mock data', | |||
There was a problem hiding this comment.
Minor, but please remove this change; we might still trigger a new renderer version because it depends on analytics-chart, but it's nice to avoid code changes when possible.
| @@ -429,6 +438,22 @@ const chartTooltipSortFn = computed(() => { | |||
| } | |||
| }) | |||
|
|
|||
| const chartHeaderPosition = computed(() => { | |||
| return props.chartTitle || !hasKebabMenuAccess || (resultSetTruncated.value && maxEntitiesShown.value) ? 'relative' : 'absolute' | |||
There was a problem hiding this comment.
resultSetTruncated && maxEntitiesShown -- I know this is the old logic, but shouldn't this be || rather than &&? We need to show a header if either of these warnings is visible? (Honest question, I may not be up to date on the designs.)
There was a problem hiding this comment.
I don't remember how this part of explore works, but may be this was based off an old faulty assumption. 😅
Those computed props are based off the limit and truncated in the explore response. Is the presence of both required to determine that the results are truncated? If not, then may be we can just use trancated ?
There was a problem hiding this comment.
Aaagh, I'm sorry. I dug a bit deeper and these two things are complementary -- basically, if results are truncated, you can read limit to render a warning message. I lost the thread and thought they were two different concepts. Technically, you could just use truncated, but the current logic is fine. No need for changes.
| @@ -364,10 +377,6 @@ const hasValidChartData = computed(() => { | |||
| return props.chartData && props.chartData.meta && props.chartData.data.length | |||
| }) | |||
|
|
|||
| const showChartHeader = computed(() => { | |||
There was a problem hiding this comment.
If a chart doesn't have a title and doesn't have valid data, does this behave the same way as before? (It seems like we're showing a header in that case where before we didn't.)
There was a problem hiding this comment.
Yes, I don't think the chart header should be hidden any longer in the case that there is no data. We may still want to show the kebab menu that could still contain valid actions a user can take, regardless of the chart having data.
For example, a chart in a dashboard may not have data for whatever reason. A user may still want to "go to explore" or "edit" the tile in order to investigate why the chart has no data, or make changes to the query backing the chart/tile. So I think the old logic had to be changed.
There was a problem hiding this comment.
I think that makes sense; my main concern is that this is changing outside of the feature flag. I took a look at the sandbox, and I think the change is basically invisible, so I won't worry about it.
| :aria-label="i18n.t('more_actions')" | ||
| class="kebab-action-menu" | ||
| data-testid="kebab-action-menu" | ||
| > |
There was a problem hiding this comment.
Good call here. Without the button element, the nested SVG can sometimes "steal" the click action
| @@ -364,10 +377,6 @@ const hasValidChartData = computed(() => { | |||
| return props.chartData && props.chartData.meta && props.chartData.data.length | |||
| }) | |||
|
|
|||
| const showChartHeader = computed(() => { | |||
There was a problem hiding this comment.
I think that makes sense; my main concern is that this is changing outside of the feature flag. I took a look at the sandbox, and I think the change is basically invisible, so I won't worry about it.
| @@ -429,6 +438,22 @@ const chartTooltipSortFn = computed(() => { | |||
| } | |||
| }) | |||
|
|
|||
| const chartHeaderPosition = computed(() => { | |||
| return props.chartTitle || !hasKebabMenuAccess || (resultSetTruncated.value && maxEntitiesShown.value) ? 'relative' : 'absolute' | |||
There was a problem hiding this comment.
Aaagh, I'm sorry. I dug a bit deeper and these two things are complementary -- basically, if results are truncated, you can read limit to render a warning message. I lost the thread and thought they were two different concepts. Technically, you could just use truncated, but the current logic is fine. No need for changes.
Summary
https://www.loom.com/share/5fa43f74fcbe4764888c49e0d1dc38ea