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

Enhance hole property in Pie Chart for variable slice radii #6769

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

barrymichaeldoyle
Copy link

Refined the Pie Chart to permit the hole property to accept both numbers and arrays. This enhancement facilitates setting individual radii for each pie slice, offering greater customization in visual representation.

Addresses: #6768

Refined the Pie Chart to permit the hole property to accept both numbers and arrays. This enhancement facilitates setting individual radii for each pie slice, offering greater customization in visual representation.
@barrymichaeldoyle
Copy link
Author

I'm not sure why the webgl-jasmine tests are failing, it seems unrelated to this PR.

@archmoj
Copy link
Contributor

archmoj commented Nov 2, 2023

I'm not sure why the webgl-jasmine tests are failing, it seems unrelated to this PR.

Yes it is unrelated to this PR and it would be fixed on master shortly.

@archmoj
Copy link
Contributor

archmoj commented Nov 2, 2023

Thanks for the PR.
Please see my comment here on how to add and handle an arrayOk attribute.

@barrymichaeldoyle
Copy link
Author

@archmoj thank you, please see my updated PR for review 👌

@barrymichaeldoyle
Copy link
Author

I think I might need some help with this.

@archmoj
Copy link
Contributor

archmoj commented Nov 7, 2023

I'm not sure why the webgl-jasmine tests are failing, it seems unrelated to this PR.

Please fetch upstream/master and merge it to this branch which would fix the webgl-jasmine test.
Thanks!

@archmoj
Copy link
Contributor

archmoj commented Nov 7, 2023

I think I might need some help with this.

After running npm start, if you run npm run schema it would update test/plot-schema.json file. Then you can commit it. That would fix publish-dist test.

src/traces/pie/plot.js Outdated Show resolved Hide resolved
@barrymichaeldoyle
Copy link
Author

I think I might need some help with this.

After running npm start, if you run npm run schema it would update test/plot-schema.json file. Then you can commit it. That would fix publish-dist test.

Awesome, I've done that, synced up to master and resolved the review item 👍

src/traces/pie/plot.js Outdated Show resolved Hide resolved
src/traces/pie/plot.js Outdated Show resolved Hide resolved
src/traces/pie/plot.js Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Nov 23, 2023

Looking good to me.
Could you please add a mock named zz-pie-hole-array.json at test/image/mocks/ with some pies to test this feature?
Thank you!

@archmoj
Copy link
Contributor

archmoj commented Dec 14, 2023

We are in the process of releasing a minor version soon.
I'd like to include this feature but the test is still missing.

@ayjayt
Copy link
Contributor

ayjayt commented Dec 29, 2023

Hey @archmoj I'd be willing to wrap up a could of PRs from third parties that interest you if a) my work is acceptable to you and b) we could talk about fast-tracking some feature additions I want to do (I have a comercial need). Thanks!

@barrymichaeldoyle
Copy link
Author

I'm sorry I've been so inactive on wrapping up this issue. I've been taking a break. I'll be back next week to tackle this. I understand if the next minor version doesn't include this feature due to my inactivity.

@archmoj
Copy link
Contributor

archmoj commented Jan 2, 2024

I'm sorry I've been so inactive on wrapping up this issue. I've been taking a break. I'll be back next week to tackle this. I understand if the next minor version doesn't include this feature due to my inactivity.

No problem. We are still in the process of working on new features for the next minor.
So we could include it when it is ready.

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

Successfully merging this pull request may close these issues.

None yet

4 participants