feat(dashboards): Add to dashboard option on dashboard widget dropdown #93922
Conversation
github-actions
bot
added
the
Scope: Frontend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #93922 +/- ##
=======================================
Coverage 88.02% 88.02%
=======================================
Files 10331 10331
Lines 596378 596374 -4
Branches 23160 23159 -1
=======================================
- Hits 524976 524973 -3
+ Misses 70946 70945 -1
Partials 456 456 |
| return ( | ||
| <GrowingWrapper> | ||
| <GrowingWrapper style={{minHeight}}> |
There was a problem hiding this comment.
when this doesn't have a min height, the number from the big number widget is reduced to a height of 0px. Let me know if there's a cleaner way to do this
There was a problem hiding this comment.
Haven't tested this (and maybe you have), but what about adding the style to GrowingWrapper directly? ex. min-height: fit-content; or something
There was a problem hiding this comment.
ya that's what I was thinking initially too but that unfortunately doesn't work like we think it would 😭
There was a problem hiding this comment.
👀 HMM. BigNumberWidgetVisualization by design obeys the height of the parent element, is it possible to set the height there, rather than adding this min-height prop? It's good to keep the prop list as small as possible!
| query: | ||
| source && source === DashboardWidgetSource.DASHBOARDS | ||
| ? { | ||
| ...convertWidgetToBuilderStateParams(widget), |
There was a problem hiding this comment.
🤔 is it possible to lift this logic up into the dashboard widget so the modal is opened with the params all ready to go? This source prop is a bit of an anti-pattern IMO, since now the modal would have to know about every possible source, rather than the sources knowing how to make queries for the modal (which would be better)
| return ( | ||
| <GrowingWrapper> | ||
| <GrowingWrapper style={{minHeight}}> |
There was a problem hiding this comment.
👀 HMM. BigNumberWidgetVisualization by design obeys the height of the parent element, is it possible to set the height there, rather than adding this min-height prop? It's good to keep the prop list as small as possible!
| @@ -78,7 +79,7 @@ export type AddToDashboardModalProps = { | |||
| router: InjectedRouter; | |||
| selection: PageFilters; | |||
| widget: Widget; | |||
| widgetAsQueryParams: WidgetAsQueryParams; | |||
| widgetAsQueryParams: WidgetAsQueryParams | WidgetBuilderStateQueryParams; | |||
There was a problem hiding this comment.
😭 the more I read about this the more confused I get! If widgetAsQueryParams is made from a widget, can't the modal just run convertWidgetToBuilderStateParams inside itself? Why pass both? This union of two objects that have nothing in common is haunted!
There was a problem hiding this comment.
ah i see what you mean, I assume we probably don't need both. I know the WidgetAsQueryParams type carries over some page level context so I can account for that when I change this. Sorry for the confusion 😭 lolll
| <WidgetCardWrapper | ||
| needsDefaultHeight={widget.displayType === 'big_number'} | ||
| > |
There was a problem hiding this comment.
Okay, I accidentally sent you on a wild goose chase. My suggestion:
- Remove this conditional, always apply the manual height
- Rename
BIG_NUMBER_WIDGET_HEIGHTtoWIDGET_PREVIEW_HEIGHT - Set
shouldResize={true}on the widget - Set
WIDGET_PREVIEW_HEIGHTto"200px"
Less code, less confusing, and the previews are nice and consistent, IMO!
nikkikapadia
deleted the
nikki/feat/add-to-dashboard-from-dashboard-widget
branch
Closes DAIN-385
Added the add to dashboard option to dashboard widgets to allow for users to add other widgets to their dashboards. Also had to make a small styling change to the big number widget so that the number shows up in the modal widget.
Screen.Recording.2025-06-20.at.11.05.04.AM.mov