-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Rework maple bars and detail charts #31
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
components/projects.js
Outdated
@@ -49,7 +49,7 @@ const Projects = () => { | |||
|
|||
return ( | |||
<> | |||
<Box sx={{ display: ['none', 'block', 'block', 'block'] }}> | |||
<Box sx={{ display: ['block', 'block', 'block', 'block'] }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looooove that we're able to show the bars on mobile now! Putting aside the expander positioning for now (which we'll also have to figure out on desktop), I think we could improve the way the labels + bars are rendered by making some mobile-specific changes:
- popping the
Badge
and the "credits [type]" labels onto separate lines so that they take up less horizontal space - aligning the maple bars to the grid, maybe 2 columns for the labels and 4 columns for the bars?
- this will require reworking the
Column
s inCategoryBar
-- lmk if I can help with that!
- this will require reworking the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these ideas!! I think I've got this behaving much better now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mobile positioning is looking sooo much better!
Two thoughts on the alignment (specifically for more, see tablet + desktop thoughts below):
- Since the triangle icon is doing something very different in the table header and the
Expander
is doing something very related in the table rows, I think anExpander
makes more sense here. - Can we align the icon element with the top of the content (aligning with the
Badge
text) instead of floating in the middle of the two charts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha yup, the triangle was an attempt to avoid any implication/confusion with math (plus sign near stacked numbers), but I agree that the triangle is not ideal here either. I'll swap back to expander. Only other thing I can think of off hand is a carrot.
components/charts/project-charts.js
Outdated
/> | ||
</Column> | ||
|
||
<Column start={1} width={[6, 8, 8, 8]}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reaction to seeing this live, especially on a larger monitor, is that the CategoryBar
s and DetailCharts
have just become too wide to be able to easily glance at (similar to the way text becomes hard to read when spread out over too many columns).
Sorry if this feels like a lot of churn! I think this was a case where seeing the real thing wired up, especially across screen sizes, was very clarifying.
My suggestions (happy to talk these through over Zoom also!):
- on screen sizes > mobile:
- restore
CategoryBar
s to their original 4-column width - render two separate
DetailCharts
, one for each bucket of credits (also 4-columns each)
- restore
- for the retirements info, maybe we can continuing to thread through the issued totals by
- continuing to use
muted
in the maple bar - continuing to use issued totals at the denominator in the
DetailsChart
- we could also play with shading in the issued totals per category as we are now (maybe lightening the shaded issued portion?), but this might be confusing since we'd no longer be labeling this on the right
- continuing to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all it's fun to try these different views out!
I tried one final (ha) attempt at a different view of the detail charts.
Now I'm showing the original view of issued/total issued on the left, and then a sort of 'zoomed in' view on the right that shows the retired/issued comparison in full width.
I feel like the proportion comparison for the second graph allows a quicker and more interesting analysis than showing the absolute numbers of the two buckets side by side while still maintaining all the overall information, but I'm still not 100% that my method is working here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha yeah sometimes with these things the "churn" is integral to the process!
A couple of reactions on this iteration:
- While I like the stacking of the bar charts on mobile, I think I prefer the side-by-side version we started with on tablet + desktop. I still findd the the maple bars are easier to glance at when they're narrower. Also the current layout can collect a lot of negative space, which not ideal for an element at the top of the tool.
- Now that we have two
DetailsChart
s side-by-side, one forissued
and one forretired / issued
, we're showing the issued amounts twice. This redundancy makes me think that maybe we can just allow the side-by-side charts to support the issued / retired comparison. If we do that, we could restore the retirementDetailsChart
to showretired_per_category / total_retired
and label with justretired_per_category
. And then we still have themuted
are of the maple bar as a visual nudge that retirements are a subset of issuances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking sooo good! Left one comment about conditionally disabling the expander, but otherwise looks good to me!
components/charts/project-charts.js
Outdated
<> | ||
<Row | ||
columns={[6, 8, 8, 8]} | ||
onClick={() => setExpanded(!expanded)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to disable this interaction when the expanded section will be empty?
theme.rawColors[COLORS[l]], | ||
0.3 | ||
)(theme)} ${retiredPercent}%, | ||
${alpha(theme.rawColors[COLORS[l]], 0.3)(theme)} ${percent}%, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looking really really nice 🤌
isLoading || error | ||
? previousCategories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super clean, very nice!
textTransform: 'uppercase', | ||
}} | ||
> | ||
<AnimateHeight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was using a negative margin that was causing this. Should be fixed now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
I've made some changes to how the maple bars are rendered to allow for easier comparison of retired/issued credits. They're now also visible on mobile.
Do the new charts feel readable? is it obvious what's going on in the detail view between retired/issued credits?
Could use some help thinking about how to display the expander... I'm worried that in this current configuration it feels like part of a math problem...