Skip to content

Conversation

@djbarnwal
Copy link
Member

@djbarnwal djbarnwal commented Apr 14, 2025

image

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@bito-code-review
Copy link

bito-code-review bot commented Apr 18, 2025

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - Bito Automatic Review Skipped - Draft PR

    Bito didn't auto-review because this pull request is in draft status.
    To trigger review, mark the PR as ready or type /review in the comment and save.
    You can change draft PR review settings here, or contact the agent instance creator at [email protected].

@bito-code-review
Copy link

bito-code-review bot commented Apr 18, 2025

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - Bito Automatic Review Skipped - Draft PR

    Bito didn't auto-review because this pull request is in draft status.
    To trigger review, mark the PR as ready or type /review in the comment and save.
    You can change draft PR review settings here, or contact the agent instance creator at [email protected].

@djbarnwal djbarnwal marked this pull request as ready for review April 21, 2025 11:54
@djbarnwal djbarnwal requested a review from ericpgreen2 April 24, 2025 09:30
@djbarnwal djbarnwal changed the title feat: allow multiple chart types, add donut feat: allow multiple chart types, add donut, heatmap Apr 24, 2025
@briangregoryholmes
Copy link
Contributor

I don't necessarily think this should hold anything up, but my impression was that we wanted to default to a donut chart visualization rather than a pie chart, which would mean our default inner radius should be something other than 0.

I also think inner radius is maybe the wrong control to expose. It should be relative to the outer radius of the pie chart, which is not user controllable and determined responsively by the size/positioning of the widget. It's more a "thickness" property that goes from, you know, 10% to 100%. You can see the issue when you set the inner radius to something large and then resize the widget container to something small.

@djbarnwal
Copy link
Member Author

I also think inner radius is maybe the wrong control to expose. It should be relative to the outer radius of the pie chart, which is not user controllable and determined responsively by the size/positioning of the widget. It's more a "thickness" property that goes from, you know, 10% to 100%. You can see the issue when you set the inner radius to something large and then resize the widget container to something small.

This makes sense. Vega doesn't support percentage values for radius out of the box but I think there's a way to have relative inner radius using expression functions. I will take this as a follow up so that this and the legend PR are unblocked.

@djbarnwal djbarnwal merged commit 5bb0898 into main Apr 29, 2025
9 checks passed
@djbarnwal djbarnwal deleted the feat/donut-chart branch April 29, 2025 10:44
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.

4 participants