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

feat(scales): Add support for symlog scale #1097

Merged
merged 9 commits into from
Sep 14, 2020
Merged

Conversation

OsQu
Copy link
Contributor

@OsQu OsQu commented Aug 10, 2020

Thank you for the great library!

Lately we needed to render some graphs using logarithmic scale, but found out that d3-scale's logarithmic scale is not very well suited for data having a wider range. For those use cases one can use symlog scale which "offers a bi-symmetric log transformation, suitable ‘for wide-range data’. Its domain can span several orders of magnitude, with negative as well as positive values." d3-scale has also API for that.

Add support for symlog scale. The code is heavily inspired by a PR adding the support for logarithmic scale: #378

Compared to logarithmic scale PR, no safe-guarding for zero or negative values is needed since the scale accepts both.

I added relevant stories to storybook but did not include links to website as I thought it's rather similar to logarithmic scale. Users browsing the storybook should find it easily though.

Here is a screenshot of a scatterplot using symlog scale:

image

Thank you in advance, and let me know if you have any suggestions for improvement.

Copy link
Contributor

@wyze wyze left a comment

Choose a reason for hiding this comment

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

Looks great Oskari! One minor casing issue to avoid PropType warning while using.

packages/scales/src/symlogScale.js Outdated Show resolved Hide resolved
@OsQu
Copy link
Contributor Author

OsQu commented Sep 14, 2020

Thank you Neil! 25 snapshot failures from CI, but looks like that the master branch is failing as well so probably unrelated to this PR.

@wyze wyze merged commit 954bef7 into plouc:master Sep 14, 2020
@wyze
Copy link
Contributor

wyze commented Sep 14, 2020

Yup. I have that fixed in #1128. Thanks for the contribution. We will get this out in the next release, which will hopefully be soon. :)

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 this pull request may close these issues.

2 participants