-
-
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
feat(dashboards): add details widget type #104059
Conversation
| options | ||
| ); | ||
| setSort(decodeSorts(config.defaultWidgetQuery.orderby), options); | ||
| } else { |
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.
Bug: DETAILS display type not reset when switching datasets
When changing datasets via SET_DATASET action while DisplayType.DETAILS is selected, the display type isn't validated against the new dataset's supportedDisplayTypes. Since DETAILS is only supported by the SPANS dataset, switching from SPANS to another dataset leaves the widget in a broken state with DETAILS display type and incompatible fields. The code handles this for ISSUE dataset (forcing TABLE) but not for DETAILS. This causes the widget to use default dataset fields instead of required DETAIL_WIDGET_FIELDS, breaking the details visualization.
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.
I tried this locally
- Select
dataset: spans - Select
type: details - Select
dataset: errors
After this the type was reset to line
static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.ts
Show resolved
Hide resolved
| state.displayType !== DisplayType.TABLE; | ||
| const isChartWidget = isChartDisplayType(state.displayType); | ||
|
|
||
| const showVisualizeSection = state.displayType !== DisplayType.DETAILS; |
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.
Bug: Sort UI not shown for DETAILS widgets
The showSortByStep condition doesn't include DisplayType.DETAILS, preventing the sort UI from appearing for DETAILS widgets. However, the SortBySelectors component explicitly handles DETAILS widgets alongside TABLE widgets, and users need sorting to control which example span appears first in the details view.
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.
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.
| return baseQuery; | ||
| } | ||
|
|
||
| export const isChartDisplayType = (displayType?: DisplayType) => { | ||
| if (!displayType) { | ||
| return true; | ||
| } | ||
| return ![DisplayType.BIG_NUMBER, DisplayType.TABLE, DisplayType.DETAILS].includes( | ||
| displayType | ||
| ); | ||
| }; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
| ? undefined | ||
| : state.limit; | ||
|
|
||
| const limit = isChartDisplayType(state.displayType) ? state.limit : undefined; |
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.
Bug: Undefined displayType handling inconsistent for limit field
The new isChartDisplayType function returns true when displayType is undefined, treating it as a chart type. However, the previous code in convertBuilderStateToWidget.ts explicitly defaulted undefined to DisplayType.TABLE via state.displayType ?? DisplayType.TABLE. This causes inconsistent behavior: when displayType is undefined, the limit field is now set to state.limit (chart behavior), but the widget's displayType property defaults to TABLE on line 77. Previously, undefined would result in limit = undefined (table behavior), matching the displayType fallback.
fullSpanDescriptioncomponent to do this, but we can iterate on this later on.detailsand test it properly (feat(dashboards): register "details" widget as valid widget on the backend #104062)isChartDisplayTypefunction. I noticed there's a TON of places that checks isChartWidget. Now that we have 3 non-chart displays i just consolidated the logic to one place.