Allow setting barchart height via props

busykomodo commented 1 year ago

Description

Problem: My users can be reliably expected to view Evidence reports on a decent-size monitor, so I typically adjust the max-width value on div.content and article in layout.svelte. This can produce charts that are 2/3x as wide as they are tall, which is a less than ideal presentation for many charts.

Solution: This PR allows report builders to specify a height in the props for the chart, supplying a chartAreaHeight as follows:

<BarChart 
    data={simple_bar} 
    x=country 
    y=value 
    xAxisTitle=Region
    chartAreaHeight=380
/>

Because of existing logic that sets chart height based on various factors, a more robust solution that considers the viewport and adjusts the aspect ratio of the chart isn't easily achieved. This may result in an imperfect mobile experience, but can be adjusted by the report designer. I believe that this approach can still be useful though despite the simplicity. The existing values have been left in place as sensible defaults so existing reports are not affected.

If the approach works for the Barchart, I'll go through and implement on the other chart types as well.

I'm not an experienced Svelte/JS dev, so feedback is welcomed.

Checklist

[x] For UI or styling changes, I have added a screenshot or gif showing before & after
[x] I have added a changeset

(I don't believe these checklist items apply to this PR).

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: 97d5629867b177c9b653741e7fb944367496f7e9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

| Name | Type | | ------------------------- | ----- | | evidence-docs | Patch | | @evidence-dev/components | Patch | | @evidence-dev/evidence | Patch | | evidence-test-environment | Patch |

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

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
evidence-development-workspace	❌ Failed (Inspect)			May 5, 2023 3:04pm
evidence-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 5, 2023 3:04pm

vercel[bot] commented 1 year ago

@antaresjhw is attempting to deploy a commit to the Evidence Dev Team on Vercel.

A member of the Team first needs to authorize it.

busykomodo commented 1 year ago

Looks like I have a linting failure - I'm not sure how to lint my project (I am using VS Code)

archiewood commented 1 year ago

Cool PR. I have some questions and thoughts on approach (rather than code!)

Why go for adjusting chartAreaHeight rather than just height?
- I would imagine users finding it easier to understand adjusting the height of the whole canvas, rather than the chart area specifically
I think if a designer is prepared to manually override the height, then they probably are happy to live with the consequences on mobile. (or at least they are making their own tradeoffs)
- I would probably add a warning about this in the docs though e.g. "Adjusting the chart height will affect all viewport sizes. This may impact the mobile experience for your users"
This PR would need both changesets and a GIF / screenshot! (There's a guide on how to add changesets above).

ItsMeBrianD commented 1 year ago

Looks like I have a linting failure - I'm not sure how to lint my project (I am using VS Code)

From the root of the project (not the example site), run pnpm run format to format the code, then pnpm run lint --fix to lint it (--fix resolves automatically fixable issues)

archiewood commented 1 year ago

~~@antaresjhw to format your code: pnpm run format~~

This time @ItsMeBrianD wins

busykomodo commented 1 year ago

Cool PR. I have some questions and thoughts on approach (rather than code!)

Why go for adjusting chartAreaHeight rather than just height? I would imagine users finding it easier to understand adjusting the height of the whole canvas, rather than the chart area specifically

Good question. The full container height is determined by adding the height of various values from elements in the header and footer of the chart. These individual elements could potentially be quite tall based on how many elements are in the legend, or even user-agent factors like their text size. It seems that a container height at the default value of 180px could potentially leave no room for the actual chart under some circumstances.

I also thought at first that being able to set a height value would be more intuitive, but as I was trying various approaches this ended up feeling like it was the most clear.

I think if a designer is prepared to manually override the height, then they probably are happy to live with the consequences on mobile. (or at least they are making their own tradeoffs) I would probably add a warning about this in the docs though e.g. "Adjusting the chart height will affect all viewport sizes. This may impact the mobile experience for your users"

The cell where this would sit typically has very concise info - I'll throw this in though and see what it looks like.

This PR would need both changesets and a GIF / screenshot! (There's a guide on how to add changesets above).

I'll go ahead and include them.

hughess commented 1 year ago

@antaresjhw thanks for this PR!

Default Height

I think the default height should be set within Chart.svelte since it will be the same for all charts. You could replace the default value in the BarChart file with chartAreaHeight = undefined.

This makes me wonder if we should offer a chartAreaHeight global setting so you could override the default height without having to define your new height in every chart you make. That would be more complicated though, so it's something we can consider in the future.

Special Situation

There is one place in the charting library where the chart area height itself is dynamically adjusted. This happens in horizontal bar charts - to maintain readability, the chart height will expand to fit the bars in the data.

@antaresjhw for your use case, would you expect setting the chartAreaHeight to override this auto-scaling behaviour for horizontal bar charts? E.g., if you had 20 bars in a horizontal bar chart, the auto-scaling would want to set the chart area height to 600px. If you've set the height at 380px, which height would you expect to see in the chart?

My gut reaction here is to go with the override - if the chart area height is supplied, use whatever value was supplied.

Note that this auto-scaling logic isn't working ideally at the moment - it's based on # of bars and is a multiple of the chart container height. I think it should be changed to be based on a specific px value for min height and compared against the chart area height. I'll make those changes in a separate PR - until they're in, horizontal bar charts will continue to auto-scale even if chartAreaHeight is supplied.

busykomodo commented 1 year ago

@hughess Thanks for the feedback and questions!

I agree with putting the default height in Chart.svelte - I was trying to do a one-off on one of the charts to make sure the general approach of the PR was OK with the team before putting it in place for all charts. If this all seems reasonable enough, I can go ahead and modify the PR to do it that way.

I think a global setting for the chart height makes sense, although I also agree that it would be a bit more complicated.

Special Situation

The height can certainly be dynamic as you call out, and I find it useful in the reports I have built so far. I guess I really don't see myself using the prop at all on a horizontal bar chart since a wide and short chart is perfectly fine here - but I didn't spend a lot of thought on the behavior for when somebody does choose to use that prop on a horizontal bar chart.

My primary goal with this PR is to avoid charts that are 4x wider than they are tall when I increase the width of my reports (on a chart-by-chart basis) - it looks awkward and less visually appealing, and it makes it harder to visually resolve differences in height i.e. when there are a bunch of values that are relatively close together.

I think there are a few options we could consider for horizontal bar charts:

Ignore the chartAreaHeight prop if supplied on a horizontal bar chart, and document that it will be ignored
Treat the chartAreaHeight prop as a minimum for the bar chart (and maybe rename it to chartAreaMinHeight)
Treat the chartAreaHeight as a firm number, which could squash a large number of rows into something not very usable.

I think I am with you on your gut reaction to treat it as an override - if it looks bad, the prop can be easily removed by the report designer. Although, the first option os probably good too?

I'll try to gather up all this feedback and get it put onto the PR over the next few days.

busykomodo commented 1 year ago

This PR now includes the following:

chartAreaHeight is exported from the Chart component, and can be set via props from supported chart types that extend it
Area, Bar, Bubble, Histogram, Line, and Scatter plots now support this prop
The chartAreaHeight value is validated, and set to a default of 180 for NaN, or negative values
I confirmed that explicitly setting a height for a horizontal bar chart overrides the calculated height that would otherwise be used
Docs have been updated for each of these chart types
Changeset generated
The Area Chart is the only one I didn't confirm directly, as that page uses BigQuery and has no charts that use the local DuckDB file.
Linting/formatting was run

I am not sure where to place the screenshot - I couldn't see instructions on where that is expected, so I placed them below:

Before:

After (chartAreaHeight=380):

netlify[bot] commented 1 year ago

Deploy Preview for evidence-development-workspace ready!

Name	Link
Latest commit	b21f28f22a256a22a3a308fd7c13aff9cbb7d7b3
Latest deploy log	https://app.netlify.com/sites/evidence-development-workspace/deploys/64551a9eeb8014000893eb2c
Deploy Preview	https://deploy-preview-809--evidence-development-workspace.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

archiewood commented 1 year ago

Okay ended up doing a bit more than I expected here.

In order to test the AreaChart, I replaced the BigQuery data, and while I was at it, purged it from the rest of the dev-workspace, replacing it with duckdb, which addresses https://github.com/evidence-dev/evidence/issues/802
DuckDB doesn't play nice with Vercel, so I've moved our dev workspace test deployment to Netlify for now

archiewood commented 1 year ago

...and moved our UI tests to reference that Netlify deployment

evidence-dev / evidence