-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(dashboards): add details widget type #104059
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.
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
Changes from all commits
1169340
1613b2d
00b713e
4858403
05675af
24b262d
289562b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import { | |
| type DashboardFilters, | ||
| type Widget, | ||
| } from 'sentry/views/dashboards/types'; | ||
| import {isChartDisplayType} from 'sentry/views/dashboards/utils'; | ||
| import {animationTransitionSettings} from 'sentry/views/dashboards/widgetBuilder/components/common/animationSettings'; | ||
| import WidgetBuilderDatasetSelector from 'sentry/views/dashboards/widgetBuilder/components/datasetSelector'; | ||
| import WidgetBuilderFilterBar from 'sentry/views/dashboards/widgetBuilder/components/filtersBar'; | ||
|
|
@@ -122,9 +123,9 @@ function WidgetBuilderSlideout({ | |
| : isEditing | ||
| ? t('Edit Widget') | ||
| : t('Custom Widget Builder'); | ||
| const isChartWidget = | ||
| state.displayType !== DisplayType.BIG_NUMBER && | ||
| state.displayType !== DisplayType.TABLE; | ||
| const isChartWidget = isChartDisplayType(state.displayType); | ||
|
|
||
| const showVisualizeSection = state.displayType !== DisplayType.DETAILS; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Sort UI not shown for DETAILS widgetsThe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional, we don't want sort UI for detail. we might want it in the future tho, so i made the sort component also work with details in this PR. |
||
|
|
||
| const customPreviewRef = useRef<HTMLDivElement>(null); | ||
| const templatesPreviewRef = useRef<HTMLDivElement>(null); | ||
|
|
@@ -340,9 +341,11 @@ function WidgetBuilderSlideout({ | |
| <WidgetBuilderFilterBar releases={dashboard.filters?.release ?? []} /> | ||
| </Section> | ||
| )} | ||
| <Section> | ||
| <Visualize error={error} setError={setError} /> | ||
| </Section> | ||
| {showVisualizeSection && ( | ||
| <Section> | ||
| <Visualize error={error} setError={setError} /> | ||
| </Section> | ||
| )} | ||
| <Section> | ||
| <WidgetBuilderQueryFilterBuilder | ||
| onQueryConditionChange={onQueryConditionChange} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import { | |
| WidgetType, | ||
| type LinkedDashboard, | ||
| } from 'sentry/views/dashboards/types'; | ||
| import {isChartDisplayType} from 'sentry/views/dashboards/utils'; | ||
| import type {ThresholdsConfig} from 'sentry/views/dashboards/widgetBuilder/buildSteps/thresholdsStep/thresholds'; | ||
| import { | ||
| DISABLED_SORT, | ||
|
|
@@ -32,12 +33,22 @@ import { | |
| DEFAULT_RESULTS_LIMIT, | ||
| getResultsLimit, | ||
| } from 'sentry/views/dashboards/widgetBuilder/utils'; | ||
| import type {DefaultDetailWidgetFields} from 'sentry/views/dashboards/widgets/detailsWidget/types'; | ||
| import {FieldValueKind} from 'sentry/views/discover/table/types'; | ||
| import {SpanFields} from 'sentry/views/insights/types'; | ||
|
|
||
| // For issues dataset, events and users are sorted descending and do not use '-' | ||
| // All other issues fields are sorted ascending | ||
| const REVERSED_ORDER_FIELD_SORT_LIST = ['freq', 'user']; | ||
|
|
||
| const DETAIL_WIDGET_FIELDS: DefaultDetailWidgetFields[] = [ | ||
| SpanFields.ID, | ||
| SpanFields.SPAN_OP, | ||
| SpanFields.SPAN_GROUP, | ||
| SpanFields.SPAN_DESCRIPTION, | ||
| SpanFields.SPAN_CATEGORY, | ||
| ] as const; | ||
|
|
||
| export const MAX_NUM_Y_AXES = 3; | ||
|
|
||
| export type WidgetBuilderStateQueryParams = { | ||
|
|
@@ -296,6 +307,16 @@ function useWidgetBuilderState(): { | |
| // Columns are ignored for big number widgets because there is no grouping | ||
| setFields([...aggregatesWithoutAlias, ...(yAxisWithoutAlias ?? [])], options); | ||
| setQuery(query?.slice(0, 1), options); | ||
| } else if (action.payload === DisplayType.DETAILS) { | ||
| setLimit(undefined, options); | ||
| setSort([], options); | ||
| setYAxis([], options); | ||
| setLegendAlias([], options); | ||
| setFields( | ||
| DETAIL_WIDGET_FIELDS.map(field => ({field, kind: FieldValueKind.FIELD})), | ||
| options | ||
| ); | ||
| setQuery(query?.slice(0, 1), options); | ||
| } else { | ||
| setFields(columnsWithoutAlias, options); | ||
| const nextAggregates = [ | ||
|
|
@@ -344,10 +365,16 @@ function useWidgetBuilderState(): { | |
| config.defaultWidgetQuery.fields?.map(field => explodeField({field})), | ||
| options | ||
| ); | ||
| if ( | ||
| nextDisplayType === DisplayType.TABLE || | ||
| nextDisplayType === DisplayType.BIG_NUMBER | ||
| ) { | ||
| if (isChartDisplayType(nextDisplayType)) { | ||
| setFields([], options); | ||
| setYAxis( | ||
| config.defaultWidgetQuery.aggregates?.map(aggregate => | ||
| explodeField({field: aggregate}) | ||
| ), | ||
| options | ||
| ); | ||
| setSort(decodeSorts(config.defaultWidgetQuery.orderby), options); | ||
| } else { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: DETAILS display type not reset when switching datasetsWhen changing datasets via
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried this locally
After this the type was reset to |
||
| setYAxis([], options); | ||
| setFields( | ||
| config.defaultWidgetQuery.fields?.map(field => explodeField({field})), | ||
|
|
@@ -359,15 +386,6 @@ function useWidgetBuilderState(): { | |
| : decodeSorts(config.defaultWidgetQuery.orderby), | ||
| options | ||
| ); | ||
| } else { | ||
| setFields([], options); | ||
| setYAxis( | ||
| config.defaultWidgetQuery.aggregates?.map(aggregate => | ||
| explodeField({field: aggregate}) | ||
| ), | ||
| options | ||
| ); | ||
| setSort(decodeSorts(config.defaultWidgetQuery.orderby), options); | ||
| } | ||
|
|
||
| setThresholds(undefined, options); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import { | |
| type Widget, | ||
| type WidgetQuery, | ||
| } from 'sentry/views/dashboards/types'; | ||
| import {isChartDisplayType} from 'sentry/views/dashboards/utils'; | ||
| import { | ||
| serializeSorts, | ||
| type WidgetBuilderState, | ||
|
|
@@ -39,7 +40,7 @@ export function convertBuilderStateToWidget(state: WidgetBuilderState): Widget { | |
| .filter(Boolean); | ||
|
|
||
| const fields = | ||
| state.displayType === DisplayType.TABLE | ||
| state.displayType === DisplayType.TABLE || state.displayType === DisplayType.DETAILS | ||
| ? state.fields?.map(generateFieldAsString) | ||
| : [...(columns ?? []), ...(aggregates ?? [])]; | ||
|
|
||
|
|
@@ -69,12 +70,7 @@ export function convertBuilderStateToWidget(state: WidgetBuilderState): Widget { | |
| }; | ||
| }); | ||
|
|
||
| const limit = [DisplayType.BIG_NUMBER, DisplayType.TABLE].includes( | ||
| state.displayType ?? DisplayType.TABLE | ||
| ) | ||
| ? undefined | ||
| : state.limit; | ||
|
|
||
| const limit = isChartDisplayType(state.displayType) ? state.limit : undefined; | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Undefined displayType handling inconsistent for limit fieldThe new Additional Locations (1) |
||
| return { | ||
| title: state.title ?? '', | ||
| description: state.description, | ||
|
|
||
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pr already up for this