Skip to content

Fixes #1324 which occurs due to a collision in the datafn. #1337

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

Closed
wants to merge 1 commit into from

Conversation

rpaskowitz
Copy link
Contributor

In the example Jan 11 is at x = 0 while Jan 1 is at x = 10. Concatenating the two results in both being "Jan 110".

Join the fields with \u0000 instead of simple concatenation to prevent the collision.

@@ -1604,7 +1604,7 @@ axes.doTicks = function(gd, axid, skipTitle) {
var axletter = axid.charAt(0),
counterLetter = axes.counterLetter(axid),
vals = axes.calcTicks(ax),
datafn = function(d) { return d.text + d.x + ax.mirror; },
datafn = function(d) { return [d.text, d.x, ax.mirror].join('\u0000'); },
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice fix 🎉

I'm curious to hear why you chose \u0000 instead of say - here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used \u0000 as a workaround but putting it on the end of our ticktext values, but not causing any visible change. No solid reason we couldn't use something else here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. In this case, I would prefer changing the join argument to - if you don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, perhaps a character that's not possible in a stringified number, like '_'? It would be rather pathological, but Imagine you had

xaxis: {
    type: 'linear',
    title: 'Grade',
    tickvals: [-1, 1],
    ticktext: ['A', 'A-']
}

(Nice debugging BTW @rpaskowitz !)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call @alexcjohnson

image

😮

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Will change.

@etpinard etpinard added status: in progress bug something broken labels Jan 30, 2017
@etpinard
Copy link
Contributor

@rpaskowitz It would be nice to add an image test for 🔒ing down this behavior. To do so, add an image test mock data/layout JSON in the test/image/mocks/ as generated in https://jsfiddle.net/hgqhrnrj/5/ If you're feeling adventurous, you can try to generate the baseline yourself in our image test docker container (docs here).

@rpaskowitz
Copy link
Contributor Author

Added a mock and tested it pre/post change and confirmed it's working as intended.

I don't think I'll be able to get to generating the baseline however. Any recommendation on adding that in a separate PR, sending me a baseline and my adding it to this PR, other...?

@etpinard
Copy link
Contributor

etpinard commented Jan 31, 2017

I don't think I'll be able to get to generating the baseline however

No worries, we'll take it from here once #1337 (comment) is addressed. Thanks again for this awesome PR!

In the example Jan 11 is at x = 0 while Jan 1 is at x = 10. Concatenating the two results in both being "Jan 110".
@etpinard
Copy link
Contributor

@rpaskowitz I added the baseline image in #1347 which includes your commit. I will close this PR and merge #1347

Thanks again 🍻

@etpinard etpinard closed this Jan 31, 2017
@etpinard etpinard added this to the 1.23.0 milestone Jan 31, 2017
@rpaskowitz rpaskowitz deleted the datafn-collision branch January 31, 2017 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants