Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: analytics chart kebab menu positioning #1785

Merged
merged 3 commits into from
Nov 19, 2024
Merged

fix: analytics chart kebab menu positioning #1785

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c837883
fix: analytics chart kebab menu positioning
filipgutica Nov 13, 2024
98223e3
fix: component test
filipgutica Nov 18, 2024
fe391ce
fix: address comments
filipgutica Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/analytics/analytics-chart/sandbox/pages/TimeSeriesChartDemo.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,6 @@
:allow-csv-export="true"
:chart-data="(exploreResult)"
:chart-options="analyticsChartOptions"
chart-title="Request count by Status Code"
:go-to-explore="(exploreLink)"
:legend-position="legendPosition"
:show-annotations="showAnnotationsToggle"
Expand Down Expand Up @@ -251,7 +250,7 @@
</template>

<script setup lang="ts">
import { computed, ref, watch, inject } from 'vue'
import { computed, ref, watch, inject, provide } from 'vue'
import {
AnalyticsChart,
ChartLegendPosition,
Expand All @@ -266,6 +265,8 @@ import { lookupStatusCodeColor } from '../../src/utils/customColors'
import type { SandboxNavigationItem } from '@kong-ui-public/sandbox-layout'
import { generateMultipleMetricTimeSeriesData, generateSingleMetricTimeSeriesData } from '@kong-ui-public/analytics-utilities'
import CodeText from '../CodeText.vue'
import { INJECT_QUERY_PROVIDER } from '../../src/constants'
import useEvaluateFeatureFlag from '../../src/composables/useEvauluateFeatureFlag'

enum Metrics {
TotalRequests = 'TotalRequests',
Expand Down Expand Up @@ -332,6 +333,7 @@ const exportCsv = () => {
}

const exploreLink: string = 'https://cloud.konghq.tech/us/analytics/explorer'
provide(INJECT_QUERY_PROVIDER, { evaluateFeatureFlagFn: () => true })

const exploreResultText = ref('')
const hasError = computed(() => !isValidJson(exploreResultText.value))
Expand Down
7 changes: 5 additions & 2 deletions packages/analytics/analytics-chart/src/components/AnalyticsChart.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the .only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wooops 🙈

cy.mount(AnalyticsChart, {
props: {
allowCsvExport: true,
Expand All @@ -316,8 +316,11 @@ describe('<AnalyticsChart />', () => {

cy.getTestId('chart-action-menu').should('exist')

cy.get('.chart-header').trigger('mouseenter')
cy.getTestId('kebab-action-menu').should('be.visible')

// eslint-disable-next-line cypress/unsafe-to-chain-command
cy.getTestId('chart-action-menu').click().then(() => {
cy.getTestId('kebab-action-menu').click().then(() => {
cy.getTestId('chart-jump-to-explore').should('not.exist')

cy.getTestId('csv-export-button').click()
Expand Down
80 changes: 68 additions & 12 deletions packages/analytics/analytics-chart/src/components/AnalyticsChart.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
<template>
<div class="analytics-chart-shell">
<div
class="analytics-chart-shell"
:class="{ 'is-hovering': isHovering }"
@mouseenter="handleMouseEnter"
@mouseleave="handleMouseLeave"
>
<div
v-if="showChartHeader"
class="chart-header"
>
<div
Expand Down Expand Up @@ -43,10 +47,17 @@
class="dropdown"
data-testid="chart-action-menu"
>
<MoreIcon
:color="KUI_COLOR_TEXT_NEUTRAL"
:size="KUI_ICON_SIZE_40"
/>
<button
appearance="none"
:aria-label="i18n.t('more_actions')"
class="kebab-action-menu"
data-testid="kebab-action-menu"
>
Copy link
Contributor

@mihai-peteu mihai-peteu Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call here. Without the button element, the nested SVG can sometimes "steal" the click action

<MoreIcon
:color="KUI_COLOR_TEXT_NEUTRAL"
:size="KUI_ICON_SIZE_40"
/>
</button>
<template #items>
<KDropdownItem
v-if="!!goToExplore"
Expand Down Expand Up @@ -158,7 +169,7 @@ import { msToGranularity } from '@kong-ui-public/analytics-utilities'
import type { AbsoluteTimeRangeV4, ExploreAggregations, ExploreResultV4, GranularityValues } from '@kong-ui-public/analytics-utilities'
import { hasMillisecondTimestamps, defaultStatusCodeColors } from '../utils'
import TimeSeriesChart from './chart-types/TimeSeriesChart.vue'
import { KUI_COLOR_TEXT_NEUTRAL, KUI_COLOR_TEXT_WARNING, KUI_ICON_SIZE_40 } from '@kong/design-tokens'
import { KUI_COLOR_TEXT_NEUTRAL, KUI_COLOR_TEXT_WARNING, KUI_ICON_SIZE_40, KUI_SPACE_70 } from '@kong/design-tokens'
import { MoreIcon, WarningIcon } from '@kong/icons'
import CsvExportModal from './CsvExportModal.vue'
import CsvExportButton from './CsvExportButton.vue'
Expand Down Expand Up @@ -249,6 +260,8 @@ const { evaluateFeatureFlag } = composables.useEvaluateFeatureFlag()
const hasKebabMenuAccess = evaluateFeatureFlag('ma-3043-analytics-chart-kebab-menu', false)

const rawChartData = toRef(props, 'chartData')
const isHovering = ref(false)


const computedChartData = computed(() => {
return isTimeSeriesChart.value
Expand Down Expand Up @@ -364,10 +377,6 @@ const hasValidChartData = computed(() => {
return props.chartData && props.chartData.meta && props.chartData.data.length
})

const showChartHeader = computed(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@filipgutica filipgutica Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return (hasValidChartData.value && resultSetTruncated.value && maxEntitiesShown.value) || props.chartTitle || (props.allowCsvExport && hasValidChartData.value)
})

const hasMenuOptions = computed(() => (props.allowCsvExport && hasValidChartData.value) || !!props.goToExplore || props.showMenu)

const timeSeriesGranularity = computed<GranularityValues>(() => {
Expand Down Expand Up @@ -429,6 +438,22 @@ const chartTooltipSortFn = computed(() => {
}
})

const chartHeaderPosition = computed(() => {
return props.chartTitle || !hasKebabMenuAccess || (resultSetTruncated.value && maxEntitiesShown.value) ? 'relative' : 'absolute'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@adorack adorack Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})

const chartHeaderWidth = computed(() => {
return chartHeaderPosition.value === 'relative' ? '100%' : KUI_SPACE_70
})

const handleMouseEnter = () => {
isHovering.value = true
}

const handleMouseLeave = () => {
isHovering.value = false
}

provide('showLegendValues', showLegendValues)
provide('legendPosition', toRef(props, 'legendPosition'))

Expand All @@ -440,8 +465,16 @@ provide('legendPosition', toRef(props, 'legendPosition'))

.analytics-chart-shell {
height: 100%;
position: relative;
width: 100%;

&.is-hovering {
.chart-header :deep(.popover-trigger-wrapper) {
opacity: 1;
visibility: visible;
}
}

.analytics-chart-parent {
height: inherit;
width: inherit;
Expand All @@ -458,6 +491,15 @@ provide('legendPosition', toRef(props, 'legendPosition'))
display: flex;
justify-content: flex-start;
padding-bottom: var(--kui-space-60, $kui-space-60);
position: v-bind('chartHeaderPosition');
right: 0;
width: v-bind('chartHeaderWidth');
z-index: 999;

&:hover :deep(.popover-trigger-wrapper) {
opacity: 1;
visibility: visible;
}

.chart-header-icons-wrapper {
display: flex;
Expand Down Expand Up @@ -493,6 +535,21 @@ provide('legendPosition', toRef(props, 'legendPosition'))
margin-left: var(--kui-space-auto, $kui-space-auto);
margin-right: var(--kui-space-0, $kui-space-0);

:deep(.popover-trigger-wrapper) {
opacity: 0;
transform: fade(0, -10px);
transition: opacity 0.3s ease, transform 0.3s ease, visibility 0.3s;
visibility: hidden;
}

.kebab-action-menu {
background: $kui-color-background-transparent;
border: none;
color: inherit;
cursor: pointer;
height: 100%;
}

li.k-dropdown-item {
a {
text-decoration: none;
Expand All @@ -512,5 +569,4 @@ provide('legendPosition', toRef(props, 'legendPosition'))
}
}
}

</style>
1 change: 1 addition & 0 deletions packages/analytics/analytics-chart/src/components/chart-types/StackedBarChart.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
>
<canvas
ref="canvas"
class="chart-canvas"
/>
</div>
</div>
Expand Down
2 changes: 2 additions & 0 deletions packages/analytics/analytics-chart/src/components/chart-types/TimeSeriesChart.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
:key="remountLineKey"
ref="chartInstance"
:chart-id="chartID"
class="chart-canvas"
:data="(chartData as any)"
data-testid="time-series-line-chart"
:options="(options as any)"
Expand All @@ -23,6 +24,7 @@
:key="remountBarKey"
ref="chartInstance"
:chart-id="chartID"
class="chart-canvas"
:data="(chartData as any)"
data-testid="time-series-bar-chart"
:options="(options as any)"
Expand Down
1 change: 1 addition & 0 deletions packages/analytics/analytics-chart/src/locales/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"debug": "debug",
"total": "Total",
"jumpToExplore": "Open in Explorer",
"more_actions": "More actions",
"chartUnits": {
"ms": "ms",
"bytes": "Byte{plural}",
Expand Down
1 change: 0 additions & 1 deletion packages/analytics/dashboard-renderer/sandbox/pages/RendererDemo.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ const dashboardConfig: DashboardConfig = {
{
definition: {
chart: {
chartTitle: 'Timeseries line chart of mock data',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type: 'timeseries_line',
},
query: {
Expand Down
Loading