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

SSR VictoryPie label position floating point precision causing discard #568

Closed
HorizonXP opened this issue Apr 22, 2017 · 2 comments · Fixed by FormidableLabs/victory-pie#146

Comments

@HorizonXP
Copy link

As you can see in the snippet below, the client renders the VictoryPie's labels with a different calculated value, which causes the server-side rendered code to be discarded. Given that this is obviously due to floating point error, is it possible to round the x and y positions to fewer decimal places to mitigate this issue? To be fair, I only saw this on Safari, and it's likely very much a fluke, but it's likely to cause issues for others in the future.

Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting. React injected new markup to compensate which works but you have lost many of the benefits of server rendering. Instead, figure out why the markup being generated is different on the client or server:
 (client) y="-95.46844641010357" data-reactid="351
 (server) y="-95.46844641010358" data-reactid="351

I'm happy to put together a PR, if I can get pointed in the right direction. My hunch is that the best spot to do the rounding is here:
https://github.com/FormidableLabs/victory-pie/blob/master/src/components/helper-methods.js#L62

Probably on the return values from the centroid calculation. Not sure if this should be made configurable via a property or not. I'm leaning to no, simply because it's unlikely that most folks need precision greater than 3 or 4 decimal places when rendering, but happy to discuss.

Thanks!

@boygirl
Copy link
Contributor

boygirl commented Apr 28, 2017

@HorizonXP yes, I would accept a PR that reduces the precision on these values. We are already rounding path values on the slices to prevent sub-pixel rendering, so we don't need to support extreme precision.

@egid
Copy link

egid commented May 8, 2017

@HorizonXP I'm running into this right now on a project as well - any objections to me jumping on the PR? If you already have work going, I don't want to step on toes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants