evidence-dev / evidence

Business intelligence as code: build fast, interactive data visualizations in pure SQL and markdown
https://evidence.dev
MIT License
3.44k stars 167 forks source link

feat: add grid #1493

Closed ItsMeBrianD closed 4 months ago

ItsMeBrianD commented 4 months ago

Description

image

Resolves #716

<Grid cols=2 gapSize="md">
    <!-- 2x2 grid of barcharts -->
    <BarChart ... />
    <BarChart ... />
    <BarChart ... />
    <BarChart ... />
</Grid>

Checklist

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 14ce7da5694a33e31fb602887825f14b0b609a59

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

This PR includes changesets to release 4 packages | Name | Type | | ----------------------------- | ----- | | @evidence-dev/core-components | Patch | | @evidence-dev/evidence | Patch | | @evidence-dev/components | 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

netlify[bot] commented 4 months ago

Deploy Preview for evidence-development-workspace ready!

Name Link
Latest commit 14ce7da5694a33e31fb602887825f14b0b609a59
Latest deploy log https://app.netlify.com/sites/evidence-development-workspace/deploys/65a9b4f9d5136900071e5061
Deploy Preview https://deploy-preview-1493--evidence-development-workspace.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

vercel[bot] commented 4 months ago

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2024 11:36pm
mcrascal commented 4 months ago

I think this probably needs grid-cols-1 for < sm screen sizes

ItsMeBrianD commented 4 months ago

I think this probably needs grid-cols-1 for < sm screen sizes

That's the current behavior:

    const colClasses = Object.freeze({
        [1]: 'grid-cols-1',
        [2]: 'grid-cols-1 md:grid-cols-2',
        [3]: 'grid-cols-1 md:grid-cols-3',
        [4]: 'grid-cols-1 md:grid-cols-2 lg:grid-cols-4',
        [5]: 'grid-cols-1 md:grid-cols-2 lg:grid-cols-5',
        [6]: 'grid-cols-1 md:grid-cols-2 lg:grid-cols-6'
    });
hughess commented 4 months ago

I'm excited for this. Is it possible to stack items within a column of a grid? For example if you wanted to put 2 BigValues on the left and 1 chart on the right?

ItsMeBrianD commented 4 months ago

I'm excited for this. Is it possible to stack items within a column of a grid? For example if you wanted to put 2 BigValues on the left and 1 chart on the right?

This could be done using a div or nested Grid, but not with this component directly, as each child of the component is placed into a cell

hughess commented 4 months ago

How would you feel about adding a Group component (or some other name) which is just a div to be used inside the grid? I think stacking will be a pretty common ask.

That's the quick and dirty approach I used in Labs here: https://labs.evidence.dev/grid/#Group%20Component3

That would also let us support markdown within a Grid

mcrascal commented 4 months ago

@hughess / @ItsMeBrianD what about a <GridItem rowSpan colSpan /> which exposed a similar interface to grid-col-span and grid-row-span?

hughess commented 4 months ago

That could be nice - I ran into an issue when playing around with grids yesterday which that might solve:

I was trying to put one long chart next to 2 vertically stacked charts. Getting the height to line up was tricky since I had to set the height of the long chart to match the total height of the 2 stacked charts. I think doing it with GridItem would have let me use rowSpan=2?

mcrascal commented 4 months ago

row-span-2 will give you a div that is the correct height for that situation, see the border box below, but it won't change the behaviour of the charts.

<div class="grid grid-cols-2 gap-5">
<div class="row-span-2 border">
<BarChart data={top_decliners} x=subscriber_type y=n_trips swapXY=true/>
</div>
<LineChart data={cumulative_duration} x=date y={['n_trips', 'prior_year']} />
<LineChart data={cumulative_duration} x=date y={['n_trips', 'prior_year']} />
</div>

CleanShot 2024-01-18 at 10 01 44@2x

hughess commented 4 months ago

Ahh got it. Maybe alongside this we should add something to the charts to allow them to fill a GridItem - something like height=full, height=100%, or maybe there would be a way to detect that it's in a GridItem behind the scenes

mcrascal commented 4 months ago

Maybe alongside this we should add something to the charts to allow them to fill a GridItem - something like height=full, height=100%, or maybe there would be a way to detect that it's in a GridItem behind the scenes.

I think both could work. For the behind the scenes option we could setContext in GridItem, then detect it on every component .

Given we'd have to touch the charts + probably most components to really make GridItems work, I'd suggest we just ship this Grid as is. It's good for small multiples, two-ups etc. and I doubt we'll need to change the cols / gapSize API even if we change how it works under the hood.

We can tackle GridItem or another solution for these more complex grid layouts in a separate PR.

One other idea would be to expose a class prop on most components and merge the string into the existing tailwind classes on the component's container.

I've used component libraries that work this way, and they are nice to use and flexible. I think with examples in the docs, we could make these pretty copy/paste friendly for most scenarios, plus they are very google-able.

Full height of its parent: <BarChart class="h-full"/>

On a card: <BarChart class="rounded border shadow"/>

What do you guys think? Ship this?

hughess commented 4 months ago

Sounds good to me