-
Notifications
You must be signed in to change notification settings - Fork 272
feat: add error boundary and responsiveness to SuperChart #175
Conversation
Codecov Report
@@ Coverage Diff @@
## master #175 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 90 94 +4
Lines 1123 1157 +34
Branches 268 276 +8
=====================================
+ Hits 1123 1157 +34
Continue to review full report at Codecov.
|
Deploy preview for superset-ui ready! Built with commit 5c49d27 |
This PR is ready for review |
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.
LGTM overall! had a couple comments about possibly simplifying the API/questions about the API going forward.
|
||
export default class SuperChart extends React.PureComponent<SuperChartProps, {}> { | ||
export default class SuperChart extends React.PureComponent<Props, {}> { |
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.
Kernel
makes this a little less understandable/more complex IMO, Base
or Core
seems a little more approachable 🤔
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.
renaming to xxxCore
}; | ||
|
||
/** SuperChart Props for version 0.11 and below has chartProps */ | ||
type ClassicProps = Omit<SuperChartKernelProps, 'chartProps'> & { |
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.
do we need to support both prop types? with a breaking version couldn't we just change to the new API?
or if you want to keep them around for a version, could we instead only support the new API in this component, and add a wrapper that just spreads chartProps
to simplify this component?
* feat: update type for line chart series * fix: another place
🏠 Internal
SuperChart
class toSuperChartCore
.@vx/responsive
type declaration.🏆 Enhancements
Create new class
SuperChart
that wrapsSuperChartCore
.Create another new class
SuperChartShell
to wrapSuperChart
and provide backward-compatibility API. This can be removed in the upcoming breaking change.SuperChartShell
is exported asSuperChart
and provide 100% backward-compatibility.ErrorBoundary
, which can be disabled if needed.width
height
asnumber
or dynamic value such as100%
.chartProps
fields to top-level.Before
This behavior is still supported, but less preferred.
Enhancement#1 Support plain object as
chartProps
This format was used in many Storybooks, although it will show error once the story is converted to typescript. With this PR, the pattern is now legit.
Enhancement#2 Can list the fields as props to
SuperChart
The new preferred way, which should be more natural.
See the storybook preview. Can use knobs to adjust
width
andheight
.This one shows error boundary.