superset icon indicating copy to clipboard operation
superset copied to clipboard

fix(chart): add default format for quarter time grain

Open soniagtm opened this issue 2 years ago • 2 comments

SUMMARY

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

Before: Screenshot 2567-02-07 at 17 18 00

After: image

TESTING INSTRUCTIONS

  1. Create a time-series bar chart
  2. Set time grain to be Quarter and observe the date format

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [ ] Required feature flags:
  • [ ] Changes UI
  • [ ] Includes DB Migration (follow approval process in SIP-59)
    • [ ] 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

soniagtm avatar Feb 07 '24 10:02 soniagtm

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.

mistercrunch avatar Feb 08 '24 00:02 mistercrunch

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.

mistercrunch avatar Feb 08 '24 00:02 mistercrunch

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

Screenshot 2567-02-29 at 15 09 52

After

Screenshot 2567-02-29 at 15 11 28

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!

soniagtm avatar Feb 29 '24 08:02 soniagtm