You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2025/04/01 19:00:16 UTC
[PR] fix: Bar Chart (legacy) migration to keep labels layout [superset]
michael-s-molina opened a new pull request, #32965:
URL: https://github.com/apache/superset/pull/32965
### SUMMARY
This PR fixes the Bar Chart (legacy) migration to keep the 45° labels layout that was the default in the legacy chart. It's also important to note that the legacy chart didn't offer a control to change the layout and that's why this fix assumes that all charts of that type will have a 45° labels layout.
### TESTING INSTRUCTIONS
Run the migration and check that migrated bar charts have a 45° labels layout by default.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Bar Chart (legacy) migration to keep labels layout [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32965:
URL: https://github.com/apache/superset/pull/32965#discussion_r2023525025
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -236,6 +236,7 @@ def _pre_action(self) -> None:
self.remove_keys.add("bar_stacked")
self.data["stack"] = "Stack" if self.data.get("bar_stacked") else None
+ self.data["x_ticks_layout"] = 45
Review Comment:
### Missing explanation for hardcoded rotation angle <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Lack of explanation for the hardcoded value 45 for x_ticks_layout in the MigrateBarChart class.
###### Why this matters
Without documentation, future maintainers won't understand why this specific rotation angle was chosen or if it's a meaningful default.
###### Suggested change ∙ *Feature Preview*
# Set default x-axis label rotation to 45 degrees for better readability
self.data["x_ticks_layout"] = 45
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf62e33d-ac4c-4c15-9d12-4154efd2e53f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf62e33d-ac4c-4c15-9d12-4154efd2e53f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf62e33d-ac4c-4c15-9d12-4154efd2e53f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf62e33d-ac4c-4c15-9d12-4154efd2e53f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cf62e33d-ac4c-4c15-9d12-4154efd2e53f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2caba22c-61f8-4c4e-9bbb-b79c05c525b8 -->
[](2caba22c-61f8-4c4e-9bbb-b79c05c525b8)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Bar Chart (legacy) migration to keep labels layout [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #32965:
URL: https://github.com/apache/superset/pull/32965#issuecomment-2770430788
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/32965?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 65.84%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`c52bf26`)](https://app.codecov.io/gh/apache/superset/commit/c52bf266d15f9efc82202febfff8315c1a763f81?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1710 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #32965 +/- ##
==========================================
+ Coverage 60.48% 65.84% +5.36%
==========================================
Files 1931 550 -1381
Lines 76236 39508 -36728
Branches 8568 0 -8568
==========================================
- Hits 46114 26016 -20098
+ Misses 28017 13492 -14525
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/32965/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/32965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.37% <0.00%> (-0.79%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/32965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/32965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.89% <0.00%> (-0.91%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/32965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.84% <100.00%> (+2.34%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/32965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.44% <100.00%> (+3.80%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/32965?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Bar Chart (legacy) migration to keep labels layout [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #32965:
URL: https://github.com/apache/superset/pull/32965
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Bar Chart (legacy) migration to keep labels layout [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32965:
URL: https://github.com/apache/superset/pull/32965#issuecomment-2773339377
@mistercrunch @betodealmeida @eschutho Could you review this PR as code owners?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org