chore(legacy-preset-chart-nvd3): move bullet chart option parsing from backend to frontend #440
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/oqnr50ybv |
Codecov Report
@@ Coverage Diff @@
## master #440 +/- ##
==========================================
+ Coverage 22.20% 22.23% +0.03%
==========================================
Files 264 265 +1
Lines 6500 6520 +20
Branches 588 590 +2
==========================================
+ Hits 1443 1450 +7
- Misses 5020 5033 +13
Partials 37 37
Continue to review full report at Codecov.
|
villebro
force-pushed
the
villebro/bullet-annotations
branch
from
061672e to
8a25fbe
Compare
| */ | ||
| import { validateNumber } from '@superset-ui/validator'; | ||
|
|
||
| const tokenizeToNumericArray = value => { |
There was a problem hiding this comment.
- Perhaps a classic function and inline export is more readable here.
- (optional) This file can also be written in typescript
export function tokenizeToNumericArray(value) {
}| import { validateNumber } from '@superset-ui/validator'; | ||
|
|
||
| const tokenizeToNumericArray = value => { | ||
| if (value && value.trim()) { |
There was a problem hiding this comment.
value.trim() returns a new string and does not mutate value
if (value) {
const tokens = value.trim().split(',').map(v => v.trim());
...
}There was a problem hiding this comment.
The thought in the if-statement was merely to check that value is defined and isn't only whitespace. Could probably have been written more elegantly, will try to refactor more sensibly.
There was a problem hiding this comment.
I noticed that too, but the code doesn't actually depend on the value being trimmed. The trim() is just being used to check that it contains characters other than spaces, so I think this is fine.
| }; | ||
|
|
||
| const tokenizeToStringArray = value => { | ||
| if (value && value.trim()) { |
There was a problem hiding this comment.
value.trim() does not mutate value
return value
? value.trim().split(',').map(token => token.trim());
: null;| return null; | ||
| }; | ||
|
|
||
| const tokenizeToStringArray = value => { |
There was a problem hiding this comment.
can use inline export like above
| it('evals numeric strings properly', () => { | ||
| expect(tokenizeToNumericArray('1')).toStrictEqual([1]); | ||
| expect(tokenizeToNumericArray('1,2,3,4')).toStrictEqual([1, 2, 3, 4]); | ||
| expect(tokenizeToNumericArray(' 1, 2, 3, 4 ')).toStrictEqual([1, 2, 3, 4]); |
There was a problem hiding this comment.
Could throw some decimal values in here, if you wanna make sure people don't randomly turn parseFloat int parseInt or something silly :)
villebro
changed the title
|
@kristw I think this is ready for another round of review. |
🏠 Internal
This moves
BulletVizrange and marker parsing logic from the backend to the frontend. Once this PR is merged the relevant logic will be removed fromviz.py: https://github.com/apache/incubator-superset/blob/5d167afb9499d7ce30c7ea763b19993af347dc23/superset/viz.py#L1094-L1124Moving this will be necessary to deprecate
viz.py, as non-datasource related data processing will not be supported in the new chart data API.SCREENSHOTS
FYI @suddjian @rusackas