fix: Rename legacy line and area charts #28113
Conversation
john-bodley
force-pushed
the
john-bodley--rename-legacy-time-series-charts
branch
from
98950ba
to
5c781ea
Compare
john-bodley
requested review from
jinghua-qa,
geido,
eschutho,
rusackas and
betodealmeida
as code owners
|
/testenv up |
|
@john-bodley Ephemeral environment spinning up at http://18.237.90.157:8080. Credentials are |
john-bodley
force-pushed
the
john-bodley--rename-legacy-time-series-charts
branch
from
5c781ea
to
f2372ad
Compare
github-actions
bot
added
i18n
john-bodley
force-pushed
the
john-bodley--rename-legacy-time-series-charts
branch
from
f2372ad
to
c18a5ac
Compare
|
/testenv down |
| @@ -41,7 +41,7 @@ describe('Charts filters', () => { | |||
| }); | |||
|
|
|||
| it('should allow filtering by "Type" correctly', () => { | |||
| setFilter('Type', 'Area Chart (legacy)'); | |||
There was a problem hiding this comment.
I'm not entirely sure what the issue was here, i.e., when I renamed this to Time-series Area Chart (legacy) the test failed, even though the entry exists in the dropdown.
My hunch is, because these are listed alphabetically, that the new position was below the fold which was problematic for the Cypress test.
|
@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
changed the title
john-bodley
force-pushed
the
john-bodley--rename-legacy-time-series-charts
branch
from
c18a5ac
to
b5f18e3
Compare
john-bodley
force-pushed
the
john-bodley--rename-legacy-time-series-charts
branch
from
b5f18e3
to
ac19db4
Compare
Ruh Roh. |
john-bodley
force-pushed
the
john-bodley--rename-legacy-time-series-charts
branch
from
ac19db4
to
1aa4bb7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #28113 +/- ##
==========================================
+ Coverage 60.49% 70.04% +9.54%
==========================================
Files 1931 1933 +2
Lines 76241 76491 +250
Branches 8566 8566
==========================================
+ Hits 46122 53578 +7456
+ Misses 28015 20809 -7206
Partials 2104 2104
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@rusackas as a code owner would you mind reviewing this change? |
|
|
michael-s-molina
added
the
v4.0
|
Ephemeral environment shutdown and build artifacts deleted. |
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:
After the enablement the "Time-series" prefix was correctly dropped from the ECharts charts (to reflect their more general form),
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:
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.
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-seriesprefix. 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
AFTER
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION