-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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: Includes 90° x-axis label rotation #26207
fix: Includes 90° x-axis label rotation #26207
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26207 +/- ##
=======================================
Coverage 69.19% 69.19%
=======================================
Files 1944 1944
Lines 75927 75927
Branches 8451 8451
=======================================
Hits 52534 52534
Misses 21208 21208
Partials 2185 2185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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. Does this cover bar charts? I didn't see "bar" on the page but maybe it's called something else.
Now it does 😆 |
@@ -138,6 +138,7 @@ const config: ControlPanelConfig = { | |||
choices: [ | |||
[0, '0°'], | |||
[45, '45°'], | |||
[90, '90°'], |
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.
@michael-s-molina is the last choice assigned the default?
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.
No. The default is defined by the default
property.
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.
Sorry I misread your PR description. I initially read it as you were setting 90°
as the default as opposed to adding it as one of the default options.
@@ -184,6 +184,7 @@ const config: ControlPanelConfig = { | |||
choices: [ | |||
[0, '0°'], | |||
[45, '45°'], | |||
[90, '90°'], |
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.
Lots of duplication here.. Could we move xAxisLabelRotation
into controls.ts
?
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 is just one example of the many code duplication in the ECharts plugins. I'm planning to improve all this once the migrations are complete.
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.
If centralizing this now is easy (which I assume it should be), I'd probably just do this right away to get it out of the way. But if you prefer to do it later that's also ok.
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.
Approving to unblock, but I'd still recommend centralizing the control to leave the codebase slightly DRYer
edit: oops, I didn't notice this was already merged 😆
(cherry picked from commit 39c6488)
(cherry picked from commit 39c6488)
SUMMARY
Includes 90° x-axis label rotation by default, as opposed to requiring users to input the value, as it's frequently used for charts with many data points.
Check #26159 for more context.
TESTING INSTRUCTIONS
CI should be sufficient.
ADDITIONAL INFORMATION