You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "soniagtm (via GitHub)" <gi...@apache.org> on 2024/02/07 10:54:10 UTC
[PR] fix(chart): add default format for quarter time grain [superset]
soniagtm opened a new pull request, #27033:
URL: https://github.com/apache/superset/pull/27033
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
When we create a time-series chart with a quarter time grain, the default formatting sometimes displays the year (YYYY) at the beginning of the year, such as '2023', while the remaining quarters display the first month of the quarter, such as 'Apr' or 'Jul'. This inconsistency in formatting can make the chart less readable and less consistent. To address this issue, I've made changes to adjust the default formatting.
I'm not sure whether this solution is overly hard-coded or if there are more effective alternatives available. Please help suggest!
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
**Before:**
**After:**
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Create a time-series bar chart
2. Set time grain to be Quarter and observe the date format
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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(chart): add default format for quarter time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #27033: fix(chart): add default format for quarter time grain
URL: https://github.com/apache/superset/pull/27033
--
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(chart): add default format for quarter time grain [superset]
Posted by "soniagtm (via GitHub)" <gi...@apache.org>.
soniagtm commented on PR #27033:
URL: https://github.com/apache/superset/pull/27033#issuecomment-1970638982
Hi @mistercrunch,
Thank you for your suggestions and apologies for the delay. You're right, this approach would only be applied to ECharts, as other charts appear to utilize the smartDateFormatter.
For ECharts, I considered utilizing the **`getTimeFormatterForGranularity`** function from `superset-frontend/packages/superset-ui-core/src/time-format/TimeFormatterRegistrySingleton.ts` or simply using **`getTimeFormatter(TimeFormatsForGranularity[granularity])`** in `getXAxisFormatter`.
While this approach functions as expected for quarter time grains, it may alter the default format of other time grains (e.g., changing date from _%d_ to _%Y-%m-%d_) as shown below. I'm not sure if this modification is acceptable, or if it's better to proceed without it.
#### **Before**
<img width="1773" alt="Screenshot 2567-02-29 at 15 09 52" src="https://github.com/apache/superset/assets/96282005/16e74b94-1d8c-469d-8bb1-58e939ecdf9e">
#### **After**
<img width="1758" alt="Screenshot 2567-02-29 at 15 11 28" src="https://github.com/apache/superset/assets/96282005/832cf87b-41b7-40a0-91ef-c11611ecb67f"><br/><br/>
Perhaps if we could change default time formatting function (_smartDateFormatter_) to receive the number and the current grain, if available, it would be more generalized, as you suggested. This way, we could also apply it to ECharts by replacing the default format from _undefined_ to _smartDateFormatter_, similar to other legacy charts. However, I'm not sure how we can pass the granularity to smartDateFormatter, or more specifically to the **`multiFormatFunc`** in `superset-frontend/packages/superset-ui-core/src/time-format/factories/createMultiFormatter.ts`.
Let me know your thoughts or if you have any other suggestions on how to proceed.
Thanks in advance!
--
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(chart): add default format for quarter time grain [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27033:
URL: https://github.com/apache/superset/pull/27033#issuecomment-1933146419
I think you'd have to get into `superset-frontend/packages/superset-ui-core/src/time-format/`, and change all interaction with time formatting so that the function would end up receiving the number and the current grain if available. The abstraction is a bit heavy here with factories and singletons and registries, ...., but I'm sure there's a way to do right at this lower level.
--
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(chart): add default format for quarter time grain [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27033:
URL: https://github.com/apache/superset/pull/27033#issuecomment-1933138483
This seems to only apply to echarts, wondering if we can improve upstream / generalize so it would take effect into all charts / time formatters.
Thinking about this, it seems to me like the right approach would be for the time formatter function(s) to both receive the number to format as well as the current time granularity. Most formatters would disregard granularity, but the default smart formatter ("Adaptive formatting") would take the granularity into account.
I'm trying to remember how that all works to give some pointers.
--
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(chart): add default format for quarter time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27033:
URL: https://github.com/apache/superset/pull/27033#issuecomment-2822272312
This hasn't been touched since January, so I'll go ahead and close it. Please feel free to reopen if warranted. Thanks!
--
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(chart): add default format for quarter time grain [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27033:
URL: https://github.com/apache/superset/pull/27033#issuecomment-2569657020
This seems to have gotten stuck, and now has conflicts. I'll convert it to draft, and it may be closed eventually, but feel free to ping us or mark it "ready for review" if the conflicts are issues discussed herein get resolved.
--
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