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

fix(explore): Pie chart label formatting when series is temporal #18216

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

kgabryje
Copy link
Member

SUMMARY

When the label formatting was deciding whether to use number formatter or time formatter, it first checked if value is a number and if not, it checked if value is Date or column type is temporal. Timestamp values are numbers, so the first condition was true and number formatter was being used for timestamps. This PR switches the order of checks so that we first check if column is temporal.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: see linked issue
After:
image

TESTING INSTRUCTIONS

  1. Create a Pie chart
  2. Select a temporal column as group by
  3. Verify that label names are formatted properly

ADDITIONAL INFORMATION

  • [] Has associated issue: pie chart date format #13293
  • 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

CC @rusackas

https://app.shortcut.com/preset/story/34104/gh-13293-prod-37-pie-chart-date-format

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #18216 (9f66652) into master (0a91a68) will increase coverage by 14.40%.
The diff coverage is 100.00%.

❗ Current head 9f66652 differs from pull request most recent head 3fa517a. Consider uploading reports for the commit 3fa517a to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master   #18216       +/-   ##
===========================================
+ Coverage   51.64%   66.05%   +14.40%     
===========================================
  Files        1591     1591               
  Lines       62423    62423               
  Branches     6287     6287               
===========================================
+ Hits        32240    41234     +8994     
+ Misses      28561    19567     -8994     
  Partials     1622     1622               
Flag Coverage Δ
javascript 50.86% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/plugins/plugin-chart-echarts/src/utils/series.ts 92.95% <100.00%> (ø)
superset/config.py 91.90% <0.00%> (+0.31%) ⬆️
superset/initialization/__init__.py 90.20% <0.00%> (+1.68%) ⬆️
superset/databases/commands/exceptions.py 94.00% <0.00%> (+2.00%) ⬆️
superset/common/query_object_factory.py 89.79% <0.00%> (+2.04%) ⬆️
superset/extensions.py 93.90% <0.00%> (+2.43%) ⬆️
superset/charts/data/commands/get_data_command.py 100.00% <0.00%> (+3.70%) ⬆️
superset/views/sql_lab.py 60.68% <0.00%> (+4.13%) ⬆️
superset/charts/commands/exceptions.py 91.48% <0.00%> (+4.25%) ⬆️
superset/views/schedules.py 64.49% <0.00%> (+4.34%) ⬆️
... and 267 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a91a68...3fa517a. Read the comment docs.

@kgabryje kgabryje merged commit 37430d4 into apache:master Jan 28, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 size/XS 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants