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.
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.
1 more nit - maybe we could just pass radix 10 instead of ignoring the error?
There was a problem hiding this comment.
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.
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
changed the title
|
@kgabryje if you can re-review, that'd be awesome :) |
| @@ -0,0 +1,4 @@ | |||
| export const convertInteger = (value: string | number) => { | |||
There was a problem hiding this comment.
- 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.
@prosdev0107 checking in on these last little tweaks... otherwise, I think this one is looking good!
…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)
mistercrunch
added
🍒 1.5.2
🍒 1.5.3
🏷️ bot
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