Skip to content
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

feat(Timeseries): Respect time grain and add i18n support for ECharts #31765

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gerbermichi
Copy link
Contributor

SUMMARY

Respect time grain and add i18n support for ECharts

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

image

AFTER

image

TESTING INSTRUCTIONS

Create a Timeseries or MixedTimeseries chart and use the time grain Quarter.

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

@dosubot dosubot bot added i18n:general Related to translations viz:charts:timeseries Related to Timeseries labels Jan 9, 2025
Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Fix Detected
Functionality Missing Translation Wrapper ▹ view
Functionality Incorrect Override of Smart Date Format ▹ view

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

  • You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
  • You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
  • Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
  • Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
  • Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

  • Check out our docs on how you can make Korbit work best for you and your team.
  • Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Comment on lines 89 to 91
if (format === SMART_DATE_ID) {
return getSmartDateDetailedFormatter();
return getTimeFormatter(SMART_DATE_DETAILED_ID, timeGrain);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect Override of Smart Date Format category Functionality

Tell me more
What is the issue?

When SMART_DATE_ID format is specified, the code overrides it with SMART_DATE_DETAILED_ID, which contradicts the user's explicit format choice.

Why this matters

Users selecting SMART_DATE_ID format will unexpectedly receive a different formatting style than requested, potentially causing confusion and inconsistency in the application's behavior.

Suggested change

Respect the user's format choice by modifying the code to:

if (format === SMART_DATE_ID) {
    return getTimeFormatter(SMART_DATE_ID, timeGrain);
}
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

Comment on lines 196 to 199
dataView: {
title: t('Data View'),
lang: [t('Data View'), t('Close'), 'Refresh'],
},

This comment was marked as resolved.

@rusackas
Copy link
Member

rusackas commented Jan 9, 2025

You need to run the pre-commit hook to pass CI.

Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.

Alternatively it is possible to run pre-commit by running pre-commit manually:

pre-commit run --all-files

@rusackas rusackas added the review:checkpoint Last PR reviewed during the daily review standup label Jan 9, 2025
@villebro
Copy link
Member

villebro commented Jan 9, 2025

@gerbermichi thanks for this PR! A few questions/requests:

  1. The PR description is very light on details for all the changes in the PR. Would it be possible to cover all changes in the description to provide more context to reviewers?
  2. I see there's a change for both i18n and time grain formatting in this PR - would it be possible to split the PR into two different PRs?
  3. I know testing i18n related features can be challenging, but would it be possible to add unit tests for the time grain formatting? Adding a test that failed before but passes after the changes tends to be a great way of showing what the current issue is, how the added/changed logic addresses that, and finally protect against future regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n:general Related to translations plugins review:checkpoint Last PR reviewed during the daily review standup size/L viz:charts:timeseries Related to Timeseries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants