-
Notifications
You must be signed in to change notification settings - Fork 271
feat: add NoResultsComponent to charts #305
Conversation
Codecov Report
@@ Coverage Diff @@
## master #305 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 102 103 +1
Lines 1212 1223 +11
Branches 298 300 +2
=====================================
+ Hits 1212 1223 +11
Continue to review full report at Codecov.
|
Deploy preview for superset-ui ready! Built with commit 2886d69 |
677e806
to
9d0e2d6
Compare
|
||
const NoResultsComponent = ({ height, width }: Props) => ( | ||
<div style={generateContainerStyles(height, width)}> | ||
<div style={{ maxWidth: 800 }}> |
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.
Please extract the style constants.
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.
done
}; | ||
|
||
const NoResultsComponent = ({ height, width }: Props) => ( | ||
<div style={generateContainerStyles(height, width)}> |
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.
We can use useMemo
hook for this to avoid creating new style object every time.
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.
done
@@ -1,3 +1,5 @@ | |||
import 'core-js/stable'; |
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.
Are these needed?
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.
oops, my bad. that must've gotten carried along from when i was trying to get the storybook to work
queryData.data === null || | ||
(Array.isArray(queryData.data) && queryData.data.length === 0) | ||
) { | ||
chart = <NoResultsComponent height={height} width={width} />; |
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.
Perhaps it should take id
and className
as well?
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.
makes sense, added
9d0e2d6
to
2886d69
Compare
🏆 Enhancements
Introduces new behavior for when no queryData is passed to a Superset Chart. Currently this never happens because Superset renders it's own error message, but this will be used in the future once we allow the backend to return empty query data as valid data.
Test plan:
Add new storybook examples and unit tests
Message on a small chart:
Message on a large chart: