-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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: adding XAxis to BigNumberTrend #21577
feat: adding XAxis to BigNumberTrend #21577
Conversation
@@ -51,3 +52,29 @@ export const xAxisControlConfig = { | |||
}, | |||
default: undefined, | |||
}; | |||
|
|||
export const temporalXAxisMixin = { |
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.
copy/paste from dndGranularitySqlaControl
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 and works as expected with a minor typing related comment. I know this is out of scope for this PR, but I'd love to see us add support for at least numerical types on the x-axis.
@@ -51,3 +52,29 @@ export const xAxisControlConfig = { | |||
}, | |||
default: undefined, | |||
}; | |||
|
|||
export const temporalXAxisMixin = { |
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.
As this is a mixin, we maybe the type could be narrowed down to something like Pick[BaseControlConfig, 'label' | 'mapStateToProps']
?
Also, I'm wondering if we should make this a function getXAXisControlMixin
that takes an optional argument dataTypes: GenericDataType[]
, which if defined, would only return the requested column types? For instance, at some point in the future I'm assuming we want to support any ordinal or linear datatypes in this chart type, and in that case we would likely want to support GenericDataType.NUMERIC
and GenericDataType.TEMPORAL
.
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.
Thanks for the review and very useful case for this mixin. I changed this variable from the temporalXAxisMixin
to the temporalColumnMixin
and removed mapStateToProps
from the original one from dndGranularitySqlaControl
. I totally agree with making a function for returning specific column types from the original datasource. I will refactor this part in separate PR.
Codecov Report
@@ Coverage Diff @@
## master #21577 +/- ##
==========================================
- Coverage 66.66% 66.65% -0.01%
==========================================
Files 1794 1794
Lines 68639 68695 +56
Branches 7300 7311 +11
==========================================
+ Hits 45755 45792 +37
- Misses 21014 21034 +20
+ Partials 1870 1869 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
SUMMARY
Apply temporal XAxis to BigNumber with Trendline
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION