-
Notifications
You must be signed in to change notification settings - Fork 14k
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
fix(plugin-chart-echarts): make to allow the custome of x & y axis title margin i… #18947
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18947 +/- ##
==========================================
- Coverage 66.52% 66.51% -0.01%
==========================================
Files 1641 1645 +4
Lines 63475 63494 +19
Branches 6443 6459 +16
==========================================
+ Hits 42226 42234 +8
- Misses 19584 19590 +6
- Partials 1665 1670 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 think Chart.jsx
shouldn't be aware of such plugin specific controls. I'd suggest parsing those values in plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
(and wherever else they're used, I think Echarts Boxplot and MixedTimeseries).
You could create a helper function in plugins/plugin-chart-echarts/src/utils
that would return a parsed number or 0 if NaN
superset-frontend/plugins/plugin-chart-echarts/src/utils/helper.ts
Outdated
Show resolved
Hide resolved
@kgabryje This LGTM, but thought you might want to check the latest revisions since you'd requested changes. |
@@ -0,0 +1,5 @@ | |||
export const convertNumber = (value: string | number) => { | |||
/* eslint radix: ["error", "as-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.
1 more nit - maybe we could just pass radix 10
instead of ignoring the error?
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 was wondering the same. I think there's some chance of it parsing some implicit octal radix, but I haven't stopped to try to break it.
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.
Resolved with radix 10
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
Sorry for late response, added 2 more comments 🙂 |
@kgabryje if you can re-review, that'd be awesome :) |
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.
This needs the apache boilerplate license text like the rest of the files. Can't merge the PR without it.
@@ -0,0 +1,4 @@ | |||
export const convertInteger = (value: string | number) => { |
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.
- You need to add a license to make this PR pass the CI (just copy paste the comment from top of any js file)
- Can we we use more descriptive file names? Maybe the same as the function name
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.
@prosdev0107 checking in on these last little tweaks... otherwise, I think this one is looking good!
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!
…tle margin i… (#18947) * fix(chartviz): make to allow the custome of x & y axis title margin in chart * fix(chartviz): add eslint radix error in chart.js * fix(chartviz): change the transformProps in chart plugin & creat helper. * fix(chartviz): lint fix & chart.js back * fix(plugin-chart-echarts): make to allow the custom margin of X & y axis in BoxPlot & Mixedtimeseries charts * fix(plugin-chart-echarts): make to change changeNumber to changeInteger * fix(plugin-chart-echarts): make to add license & change file name (cherry picked from commit c79ee56)
SUMMARY
[chart viz][Time-series bar chart v2] chart will shrink when input customize X AXIS TITLE MARGIN
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
This issue happens because that the custom margin value of x & y axis title is string value.
And so I fixed the issue by converting them from string to number value.
ADDITIONAL INFORMATION