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

Use Plotly's function to clean y-values (x may be category or date/time) #2872

Merged
merged 1 commit into from Oct 11, 2018

Conversation

Projects
None yet
2 participants
@kravets-levko
Collaborator

kravets-levko commented Sep 28, 2018

(Bug reported by Redash user).

Plotly can detect and use values with some extra characters (like $6,123.88). Our custom code that extends Plotly wasn't able to use such values and therefore some functions didn't work (tooltips, stacking etc.). This PR adds support for such values (using Plotly's function to clean data).

@now

This comment has been minimized.

Show comment
Hide comment
@now

now bot Sep 28, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

now bot commented Sep 28, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@@ -36,6 +37,10 @@ export const ColorPalette = Object.assign({}, BaseColors, {
const ColorPaletteArray = values(BaseColors);
function cleanNumber(value) {
return isUndefined(value) ? value : (plotlyCleanNumber(value) || 0.0);

This comment has been minimized.

@arikfr

arikfr Sep 28, 2018

Member

What if value is not supposed to be a number? I think it will be safer to return value if plotlyCleanNumber return undefined (BADNUM).

Also why not have it as part of normalizeValue call?

Does the tooltip even take normalized values?

@arikfr

arikfr Sep 28, 2018

Member

What if value is not supposed to be a number? I think it will be safer to return value if plotlyCleanNumber return undefined (BADNUM).

Also why not have it as part of normalizeValue call?

Does the tooltip even take normalized values?

This comment has been minimized.

@kravets-levko

kravets-levko Sep 28, 2018

Collaborator

Generally, all y-values (y, y-error, size) should be numbers; it means that actually we don't need to use normalizeValue there (noticed it after your comment), but just cleanNumber; in opposite, x-values may be any (date/time, category, number), so we should sanitize them with normalizeValue, but not with cleanNumber (it's not important for x-values because we dont do any computing with them - just use as-is; but we use y-values to implement stacking (line/area chars) and percent normalization, so it should be a number).

@kravets-levko

kravets-levko Sep 28, 2018

Collaborator

Generally, all y-values (y, y-error, size) should be numbers; it means that actually we don't need to use normalizeValue there (noticed it after your comment), but just cleanNumber; in opposite, x-values may be any (date/time, category, number), so we should sanitize them with normalizeValue, but not with cleanNumber (it's not important for x-values because we dont do any computing with them - just use as-is; but we use y-values to implement stacking (line/area chars) and percent normalization, so it should be a number).

This comment has been minimized.

@kravets-levko

kravets-levko Oct 3, 2018

Collaborator

@arikfr Updated the PR: there is the only chart type where y may be non-number - it's bubble chart; for other types non-numeric y does not make any sense.

@kravets-levko

kravets-levko Oct 3, 2018

Collaborator

@arikfr Updated the PR: there is the only chart type where y may be non-number - it's bubble chart; for other types non-numeric y does not make any sense.

@kravets-levko kravets-levko changed the title from Use Plotly's function to clean y-values (x may be category or date/time) to (WIP) Use Plotly's function to clean y-values (x may be category or date/time) Sep 29, 2018

@kravets-levko kravets-levko changed the title from (WIP) Use Plotly's function to clean y-values (x may be category or date/time) to Use Plotly's function to clean y-values (x may be category or date/time) Oct 3, 2018

@kravets-levko kravets-levko changed the title from Use Plotly's function to clean y-values (x may be category or date/time) to (WIP) Use Plotly's function to clean y-values (x may be category or date/time) Oct 3, 2018

@kravets-levko kravets-levko changed the title from (WIP) Use Plotly's function to clean y-values (x may be category or date/time) to Use Plotly's function to clean y-values (x may be category or date/time) Oct 3, 2018

@arikfr arikfr merged commit 9b59d10 into getredash:master Oct 11, 2018

3 checks passed

WIP ready for review
Details
ci/circleci: unit-tests Your tests passed on CircleCI!
Details
codeclimate All good!
Details
@arikfr

This comment has been minimized.

Show comment
Hide comment
@arikfr

arikfr Oct 11, 2018

Member

👍

Member

arikfr commented Oct 11, 2018

👍

yoavbls added a commit to yoavbls/redash that referenced this pull request Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment