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

Half Donut Vertical Centering #444

Closed
wants to merge 10 commits into from
Closed

Half Donut Vertical Centering #444

wants to merge 10 commits into from

Conversation

curran
Copy link

@curran curran commented Sep 5, 2024

Closes #441

@curran
Copy link
Author

curran commented Sep 5, 2024

Current status:

image

@curran curran mentioned this pull request Sep 5, 2024
2 tasks
@curran
Copy link
Author

curran commented Sep 11, 2024

Current status: This PR makes the half donut centered vertically.

image

I think this is ready for review and testing. Thanks!

@curran curran marked this pull request as ready for review September 11, 2024 16:00
@curran curran changed the title WIP Half Donut Half Donut Vertical Centering Sep 11, 2024
@curran
Copy link
Author

curran commented Sep 11, 2024

Direction forward:

  • Modify the examples to showcase the responsive behavior of all pie examples when resizing the SVG both vertically and horizontally
  • Compute the bounding box that surrounds the donut
  • Use the bounding box for the scaling logic to fill the available space in the SVG

@curran curran marked this pull request as draft September 11, 2024 19:04
@curran
Copy link
Author

curran commented Oct 10, 2024

Current status:

  • Got the resizing and scaling behavior to a point where it feels right
  • Scaffolded the scaling and translation logic for all 4 types of half-donut (but haven't tested those yet - I'm debating whether I should create a new example for each case, or maybe just spot check them in development)
halfDonutResizeDemoSmall.mov
  • Problem with this: the sub-label gets cut off, because it's below the edge of the half donut
  • Proposed solution: we allow configuration of the margin (called bleed in this codebase) from the example
  • If we allow configuration of the margin, then the example could set a bottom margin to account for the space needed by the sub-label

I propose to make it configurable like this:

      <VisDonut
        ...
        bleed={{ top: 0, bottom: 0, left: 0, right: 0 }} // Current not available as a prop
      />

What do you think of this idea to expose the bleed as a prop @rokotyan ? Thanks!

@rokotyan
Copy link
Contributor

What do you think of this idea to expose the bleed as a prop @rokotyan ? Thanks!

All other Unovis components calculate the bleed value automatically, so exposing it as a prop goes a bit against the philosophy of the library. Also, we already have container properties like margin and padding, and I think adding bleed might make it more confusing for users.

Scaffolded the scaling and translation logic for all 4 types of half-donut (but haven't tested those yet - I'm debating whether I should create a new example for each case, or maybe just spot check them in development)

You can have all 4 of them on one page. This way it'll also be easier to test them with a visual testing framework.

@curran
Copy link
Author

curran commented Oct 10, 2024

we already have container properties like margin and padding

Oh that's excellent news! I didn't think the margin was already configurable, since I didn't see it in DonutConfigInterface or its superclass ComponentConfigInterface. I did not think to look in the VisSingleContainer props, but sure enough it's there and it works like a charm:

image

Thank you for the tip!

@curran
Copy link
Author

curran commented Oct 10, 2024

Current status:

  • Got a setup working where I can test all the various half-donut cases. Here's what they look like:
image image image image
  • Idea: export the angle ranges from Unovis somehow. WDYT @rokotyan ? Something like this:
export const halfDonutTopAngleRange: [number, number] = [-1.5707963267948966, 1.5707963267948966]
export const halfDonutRightAngleRange: [number, number] = [0, 3.141592653589793]
export const halfDonutBottomAngleRange: [number, number] = [1.5707963267948966, 4.71238898038469]
export const halfDonutLeftAngleRange: [number, number] = [3.141592653589793, 6.283185307179586]

Not sure the best way to do that within the Unovis setup... Attach them as properties of VisDonut maybe?

e.g.

import { VisDonut } from '@unovis/react'

...

      <VisDonut
        angleRange={VisDonut.halfDonutTopAngleRange}
      />

Another option would be:

import { halfDonutTopAngleRange } from '@unovis/ts'

...

      <VisDonut
        angleRange={halfDonutTopAngleRange}
      />

or something like that. Curious to know what the idiomatic Unovis API design would look like for something like this.

  • Working through getting the scaling right for all cases.
  • I do like the idea of a big example page with all the options, but that approach makes it more difficult to test out the responsive scaling as I go

Plan for next steps:

  • Get the scaling to work properly, each possibility in isolation
  • Make a new example page that shows all the possible half-donut configurations in one place (likely without nice responsive behavior)
  • Iterate once more on the PR based on feedback, then I think this will be ready to go!

@rokotyan
Copy link
Contributor

@curran
Instead of forcing the users to use margin, I would change the text baseline and alignment and of the labels automatically to always stay visible, or make it configurable at least.

For example:
image

As for exporting the constants, we can add a constants.ts file to components/donut and re-export it from index.ts. The only thing, I would like to prefix the constants with the component name and maybe use the UPPER_CASE. i.e. DONUT_HALF_ANGLE_RANGE_TOP instead of halfDonutTopAngleRange.

@curran
Copy link
Author

curran commented Oct 11, 2024

Excellent! Thank you for this feedback. I will do it that way.

  • Donut half right: align text to start
  • Donut half left: align text to end
  • Donut half top: offset text vertically (upwards) to fit the sub-label within the bounds of the half donut rectangle
  • Donut half bottom: offset text vertically (downwards) to fit the sub-label within the bounds of the half donut rectangle

@curran
Copy link
Author

curran commented Oct 11, 2024

Finally nailed the scaling for top half donut!

halfDonutResizing.mov

@curran
Copy link
Author

curran commented Dec 6, 2024

Closing in favor of

@curran curran closed this Dec 6, 2024
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.

Refinements for Half Donut
2 participants