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

RadialBar: Utilize defs/fill, modeled after Bar implementation #1957

Merged
merged 6 commits into from
May 10, 2022

Conversation

bdefore
Copy link
Contributor

@bdefore bdefore commented Mar 23, 2022

I wanted to use the patterned fill seen in https://nivo.rocks/marimekko/ for my use of ResponsiveRadialBar but noticed that defs and fill props were not expected in the RadialBarSvgProps type nor the implementation to pass them along to SvgWrapper in RadialBar the way that Bar does. This PR adds that implementation in a similar way.

Intermediate commits show I was initially pursuing this as Bar does by having bindKeys do a targetKey of data.fill (https://github.com/plouc/nivo/blob/master/packages/bar/src/Bar.tsx#L268) but it felt more appropriate to instead avoid polluting incoming data and instead target fill at the root ComputedBar. I'd be fine with either approach if one is preferred.

Note that the test suite fails on packages/scatterplot/tests/ScatterPlot.test.tsx but I believe these failures predate this proposal.

image

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 23, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b6e9922:

Sandbox Source
nivo Configuration

@plouc
Copy link
Owner

plouc commented May 10, 2022

@bdefore, thank you for this addition, it makes sense to do it the way you did, and it works with the arcs package!

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