Add pie chart for asset/debt allocation in dashboard view #666

Merged
JoshAntBrown merged 4 commits from feature/d3-pie-chart into main 2024-04-24 00:05:18 +08:00
JoshAntBrown commented 2024-04-23 05:51:52 +08:00 (Migrated from github.com)

Demo Video:

https://github.com/maybe-finance/maybe/assets/1793797/fd428f4e-e656-49fe-9e6e-05b573ada216

I tried to keep this pretty close to how the time series chart was implemented for consistency but I ended up having to take a different route in a couple of places. Specifically the view box of the SVG in the pie chart is fixed instead of being dynamically calculated on mount, this allows the thickness of the ring to scale according to the width of the pie chart and avoids the issue of one of the pie charts having a 0 width due to being hidden in another tab.

I opted to keep colors using the existing tailwind mechanisms we have right now this means we need to pass both a fill_color and a bg_color to each value in the pie chart since both are used to render the chart. There doesn't seem to be any mechanism to get the raw color code for an accountable right now, but let me know if you'd like to go down that approach instead.

/claim #644
Resolves #644

Demo Video: https://github.com/maybe-finance/maybe/assets/1793797/fd428f4e-e656-49fe-9e6e-05b573ada216 I tried to keep this pretty close to how the time series chart was implemented for consistency but I ended up having to take a different route in a couple of places. Specifically the view box of the SVG in the pie chart is fixed instead of being dynamically calculated on mount, this allows the thickness of the ring to scale according to the width of the pie chart and avoids the issue of one of the pie charts having a 0 width due to being hidden in another tab. I opted to keep colors using the existing tailwind mechanisms we have right now this means we need to pass both a fill_color and a bg_color to each value in the pie chart since both are used to render the chart. There doesn't seem to be any mechanism to get the raw color code for an accountable right now, but let me know if you'd like to go down that approach instead. /claim #644 Resolves #644
zachgoll (Migrated from github.com) reviewed 2024-04-23 22:12:57 +08:00
zachgoll (Migrated from github.com) left a comment

@JoshAntBrown code looks good! In regards to the accountable colors, I think that's something I've just been trying to defer as long as possible until we really start diving into and implementing that inheritance hierarchy, so I think what we've got here is good for now.

@JoshAntBrown code looks good! In regards to the accountable colors, I think that's something I've just been trying to defer as long as possible until we really start diving into and implementing that inheritance hierarchy, so I think what we've got here is good for now.
zachgoll commented 2024-04-23 22:16:34 +08:00 (Migrated from github.com)

@justinfar I'm not really sure what the correct way to go on this is, so punting this question over to you.

In regards to the sizing of the pie chart and the line chart in the dashboard, a couple questions:

  1. While we're not concerned about mobile designs, each device will have a slightly different size, which means that there could be some spacing underneath the pie chart depending on the line chart height. How do you want this to scale? Should the pie chart grow to fill its container and the line chart shrink in width? The opposite?
  2. As @JoshAntBrown has implemented, the thickness of the pie chart line will scale up and down. Is this correct behavior?
@justinfar I'm not really sure what the correct way to go on this is, so punting this question over to you. In regards to the _sizing_ of the pie chart and the line chart in the dashboard, a couple questions: 1. While we're not concerned about mobile designs, each device will have a slightly different size, which means that there could be some spacing underneath the pie chart depending on the line chart height. How do you want this to scale? Should the pie chart grow to fill its container and the line chart shrink in width? The opposite? 2. As @JoshAntBrown has implemented, the thickness of the pie chart line will scale up and down. Is this correct behavior?
justinfar commented 2024-04-23 22:47:45 +08:00 (Migrated from github.com)

@zachgoll

  1. Is it possible to set a max-height on the height of the line chart/NW card so that they're inline with each other in terms of size?

  2. I'm good with dynamically scaling the pie chart thickness slightly for different screen sizes, although in the initial video I'm noticing that the thickness of the donut chart's segments are slightly thicker than in the design (was this done mainly due to increase the size of the hover targets?)

@zachgoll 1. Is it possible to set a max-height on the height of the line chart/NW card so that they're inline with each other in terms of size? 2. I'm good with dynamically scaling the pie chart thickness slightly for different screen sizes, although in the initial video I'm noticing that the thickness of the donut chart's segments are slightly thicker than in the design (was this done mainly due to increase the size of the hover targets?)
JoshAntBrown commented 2024-04-23 22:56:44 +08:00 (Migrated from github.com)

I'm good with dynamically scaling the pie chart thickness slightly for different screen sizes, although in the initial video I'm noticing that the thickness of the donut chart's segments are slightly thicker than in the design (was this done mainly due to increase the size of the hover targets?)

Happy to decrease the thickness, but yeah it felt just a little bit too fiddly to hover on especially with them being curved. Tried to increase them without them feeling overly thick.

> I'm good with dynamically scaling the pie chart thickness slightly for different screen sizes, although in the initial video I'm noticing that the thickness of the donut chart's segments are slightly thicker than in the design (was this done mainly due to increase the size of the hover targets?) Happy to decrease the thickness, but yeah it felt just a little bit too fiddly to hover on especially with them being curved. Tried to increase them without them feeling overly thick.
justinfar commented 2024-04-23 23:08:56 +08:00 (Migrated from github.com)

I'm good with dynamically scaling the pie chart thickness slightly for different screen sizes, although in the initial video I'm noticing that the thickness of the donut chart's segments are slightly thicker than in the design (was this done mainly due to increase the size of the hover targets?)

Happy to decrease the thickness, but yeah it felt just a little bit too fiddly to hover on especially with them being curved. Tried to increase them without them feeling overly thick.

All good with the thickness was just noting it down to make sure :)

> > I'm good with dynamically scaling the pie chart thickness slightly for different screen sizes, although in the initial video I'm noticing that the thickness of the donut chart's segments are slightly thicker than in the design (was this done mainly due to increase the size of the hover targets?) > > Happy to decrease the thickness, but yeah it felt just a little bit too fiddly to hover on especially with them being curved. Tried to increase them without them feeling overly thick. All good with the thickness was just noting it down to make sure :)
JoshAntBrown commented 2024-04-23 23:43:19 +08:00 (Migrated from github.com)
Screenshot 2024-04-23 at 16 39 52

The net worth chart was set to height 100% which was causing additional space (since the container is shared with the header above). Adding the pie chart further increased the initial height of the element causing the chart to be larger than before. I've updated the net worth chart to fill the remaining height with flexbox instead and set a min height to ensure the chart is always visible.

<img width="1074" alt="Screenshot 2024-04-23 at 16 39 52" src="https://github.com/maybe-finance/maybe/assets/1793797/5c579ab4-f186-43a6-9f19-6efe9e7f7535"> The net worth chart was set to height 100% which was causing additional space (since the container is shared with the header above). Adding the pie chart further increased the initial height of the element causing the chart to be larger than before. I've updated the net worth chart to fill the remaining height with flexbox instead and set a min height to ensure the chart is always visible.
zachgoll (Migrated from github.com) approved these changes 2024-04-24 00:04:15 +08:00
zachgoll (Migrated from github.com) left a comment

Think we're all set with this one! Nice work, this looks great.

Think we're all set with this one! Nice work, this looks great.
Sign in to join this conversation.