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: Rename legacy line and area charts #28113

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Apr 17, 2024

SUMMARY

Prior to the enablement of the generic chart x-axis, the relevant ECharts charts included the "Time-series" prefix whereas several of the deprecated NVD3 charts didn't:

Screenshot 2024-04-17 at 8 53 53 AM

After the enablement the "Time-series" prefix was correctly dropped from the ECharts charts (to reflect their more general form),

Screenshot 2024-04-17 at 8 54 04 AM

however this resulted in a somewhat of a convoluted UX, i.e., the legacy NVD3 charts only work when the x-axis is a time-series (temporal) and thus I thought the names should be updated to reflect this.

This PR renames the following NVD3 chart types:

  • Area Chart (legacy) → Time-series Area Chart (legacy)
  • Line Chart (legacy) → Time-series Line Chart (legacy)

It's worth noting that the the legacy NVD3 temporal bar chart was already named appropriately given it needed to distinguish itself from the legacy NVD3 non-temporal bar chart.

Screenshot 2024-04-17 at 2 54 41 PM

Finally I haven't changed any of the translations, but the keys have been updated so translations will still exist even though the don't include the Time-series prefix. I looked into updating these via Google Translate, but I'm not entirely sure how accurate they are.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

Screenshot 2024-04-18 at 8 30 27 AM

AFTER

Screenshot 2024-04-18 at 8 30 42 AM

TESTING INSTRUCTIONS

CI.

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

@john-bodley
Copy link
Member Author

/testenv up

Copy link
Contributor

@john-bodley Ephemeral environment spinning up at http://18.237.90.157:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@john-bodley john-bodley force-pushed the john-bodley--rename-legacy-time-series-charts branch from 5c781ea to f2372ad Compare April 19, 2024 04:31
@github-actions github-actions bot added i18n Namespace | Anything related to localization i18n:spanish Translation related to Spanish language i18n:italian Translation related to Italian language i18n:french Translation related to French language i18n:chinese Translation related to Chinese language i18n:japanese Translation related to Japanese language i18n:russian Translation related to Russian language i18n:korean Translation related to Korean language i18n:dutch i18n:slovak i18n:ukrainian i18n:portuguese i18n:brazilian labels Apr 19, 2024
@john-bodley john-bodley force-pushed the john-bodley--rename-legacy-time-series-charts branch from f2372ad to c18a5ac Compare April 19, 2024 16:03
@michael-s-molina
Copy link
Member

@michael-s-molina would you mind taking a look at this? Note it's listed as a chore, but I'm not certain whether in fact it would be considered a fix for 4.0 given it ensures parity with how other legacy NVD3 charts are named within the generic chart x-axis realm.

I think you can consider this as a fix.

@john-bodley john-bodley changed the title chore: Rename legacy line and area charts fix: Rename legacy line and area charts Apr 25, 2024
@john-bodley john-bodley force-pushed the john-bodley--rename-legacy-time-series-charts branch from c18a5ac to b5f18e3 Compare April 27, 2024 04:32
@john-bodley john-bodley force-pushed the john-bodley--rename-legacy-time-series-charts branch from b5f18e3 to ac19db4 Compare April 27, 2024 04:42
@rusackas
Copy link
Member

rusackas commented Apr 29, 2024

{'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 2623 seconds.', code='throttled')}{'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 0 seconds.', code='throttled')}{'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 978 seconds.', code='throttled')}{'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 3599 seconds.', code='throttled')}{'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 3599 seconds.', code='throttled')}{'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 3598 seconds.', code='throttled')}

Ruh Roh.

@john-bodley john-bodley force-pushed the john-bodley--rename-legacy-time-series-charts branch from ac19db4 to 1aa4bb7 Compare May 1, 2024 20:12
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.04%. Comparing base (76d897e) to head (1aa4bb7).
Report is 1094 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #28113      +/-   ##
==========================================
+ Coverage   60.48%   70.04%   +9.55%     
==========================================
  Files        1931     1933       +2     
  Lines       76236    76491     +255     
  Branches     8568     8566       -2     
==========================================
+ Hits        46114    53578    +7464     
+ Misses      28017    20809    -7208     
+ Partials     2105     2104       -1     
Flag Coverage Δ
hive 49.21% <ø> (+0.04%) ⬆️
javascript 57.72% <ø> (+0.01%) ⬆️
mysql 77.54% <ø> (?)
postgres 77.67% <ø> (?)
presto 53.83% <ø> (+0.03%) ⬆️
python 83.26% <ø> (+19.78%) ⬆️
sqlite 77.14% <ø> (?)
unit 57.72% <ø> (+0.09%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@john-bodley
Copy link
Member Author

@rusackas as a code owner would you mind reviewing this change?

@rusackas
Copy link
Member

rusackas commented May 2, 2024

@michael-s-molina michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label May 2, 2024
@michael-s-molina michael-s-molina merged commit b4c4ab7 into apache:master May 2, 2024
29 checks passed
Copy link
Contributor

github-actions bot commented May 2, 2024

Ephemeral environment shutdown and build artifacts deleted.

@john-bodley john-bodley deleted the john-bodley--rename-legacy-time-series-charts branch May 2, 2024 20:16
dpgaspar pushed a commit to preset-io/superset that referenced this pull request May 6, 2024
jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
@mistercrunch mistercrunch added 🍒 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 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 i18n:brazilian i18n:chinese Translation related to Chinese language i18n:dutch i18n:french Translation related to French language i18n:italian Translation related to Italian language i18n:japanese Translation related to Japanese language i18n:korean Translation related to Korean language i18n:portuguese i18n:russian Translation related to Russian language i18n:slovak i18n:spanish Translation related to Spanish language i18n:traditional-chinese i18n:ukrainian i18n Namespace | Anything related to localization plugins size/L v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch 🍒 4.0.2 🚢 4.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants