Closed hughess closed 1 year ago
Latest commit: 00f097f585abaaa11c8a077a49230304f1e3fc8d
The changes in this PR will be included in the next version bump.
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Updated |
---|---|---|---|
evidence-docs | ✅ Ready (Inspect) | Visit Preview | Aug 31, 2022 at 9:30PM (UTC) |
A problem introduced with the 100% limit on the y-axis is an overlap with the y-axis title (if one is applied). I don't have a clear idea of a fix for this yet:
@hughess I think this is probably the wrong placement for this y axis label anyway. I think a more natural position would be above the y-axis, or on the LHS. Realize this might take a bit of thinking due to title / subtitle position
Interestingly FT and Economist both put y axis info in the subtitle
(And axis labels on RHS):
NYT also puts y axis title in subtitle position
@archiewood those are interesting examples. I agree about considering some other placement.
Given that moving the label would be reasonably time-consuming (because we'll need to test it out across all the chart types/orientations), I'm wondering if we should release the stacked charts despite this issue, or wait to release them until we have the issue solved.
I'm thinking that a stacked % chart would rarely need a y-axis label, so I'm leaning towards releasing the charts now and fixing the label issue later. What do you think?
I think the only time you would need one would be something a bit descriptive, like "% of total orders" or similar.
I think it's probably fine as you say. If someone really wants to label the y axis, they can use the subtitle.
You could put a note in the docs about it, but might just be confusing.
Another issue with the current approach is that the tooltip values are not able to receive the correct percentage formatting.
This is because all the aggregation logic is located inside of Bar.svelte
.
Chart
parent component
Bar
to use in the chart, then overriding the config of the chart before it's rendered. The new column gets passes through correctly, but the column name and formatting don't make it through. I've tried getting Bar.svelte
to update the $props
store used in Chart.svelte
, but to no avail. The simplest solution to this issue for now is to move the aggregation logic from the low-level Bar
component into the high-level BarChart
component, so the data is transformed before Chart
gets called. This should cover most of our use cases for 100% stacked charts, but it also means that the 100% stack will not work in composable charts.
i.e., the below would not work:
<Chart data={sales_data} x=category y=sales>
<Bar type=stacked100/>
</Chart>
...but this would:
<BarChart data={sales_data} x=category y=sales type=stacked100/>
I love the stacked chart. Can't wait to use it in my reports.
Thoughts:
x
and y
props for it to work. This is not how the normal stacked bar works:
<BarChart
data={orders_by_category}
series=category
/>
works
But
<BarChart
data={orders_by_category}
series=category
type=stacked100
/>
does not. And yields a rather puzzling error:
example-project
these don't look great. But this is a more general problem in Longer term I agree with some of your ideas. The only one i would add is having it so that the Chart doesn't take up 100% width, though this might have wider implications!
@archiewood thanks! Great catch. That's a bug caused by putting the aggregation in the top-level component - will see how we can get the same prop-filling as the regular stacked bar.
This has uncovered another similar issue - the stacked100 option doesn't work in charts where multiple y columns are supplied (either explicitly or assumed by Chart
through it's prop-filling logic).
There are 2 steps to fix this:
BarChart
and AreaChart
components into Chart
, so we can trigger the aggregation after the props have all been filled ingetStackedPercentages
function to allow it to handle multiple y columns and output the appropriate dataset to use in the chartStep 1 is done. Step 2 I'll tackle tomorrow.
@archiewood this last commit should fix the error you had. Would you mind giving it a quick test when you have a chance, just to make sure?
Yeah this works as I expected:
Code:
```needful_items
select
substr(order_datetime,0,7) as order_month,
item,
sum(sales) as sales_usd
from orders
group by 1,2 order by 1
<BarChart data={needful_items} series=item sort=false />
<BarChart data={needful_items} series=item sort=false type=stacked100 />
Result:
<img width="609" alt="image" src="https://user-images.githubusercontent.com/58074498/187786966-5bcdb1be-bef9-40d6-891f-0f7daa73e934.png">
Amazing, thanks!
This PR implements 100% stacked bar and area charts using the type prop (type=stacked100).
How It Works
Other Changes
yMax
: set the maximum value of the y-axis. Helpful if you are passing your own 100% stacked data to a normal bar chartyAxisLabels
andxAxisLabels
: allow you to turn on or off labels on the x and y axisCurrent Limitations
BarChart
andAreaChart
and does not work in composable chartsy
column has a format tag included in it (e.g.,sales_usd
), the column name resulting from the aggregation will append another format tag to the end for percentage (e.g.,sales_usd_pct
--->Sales Usd (%)
)