wadackel / react-stack-grid

Pinterest like layout components for React.js
https://wadackel.github.io/react-stack-grid/
MIT License
870 stars 77 forks source link

Calculating Item Heights for Nodes with Overflow [feature request] #35

Closed solomonhawk closed 2 years ago

solomonhawk commented 7 years ago

Hey there,

Thanks for making this!

There is an assumption in your getItemHeight function that creates an inflexibility around some use cases which I will attempt to describe.

In cases where an element has overflowing children - regardless of what the parent's overflow value is set to, this calculation results in a value that accounts for the children's full dimensions (exceeding the parent's dimensions). This does make sense.. if I have a div whose overflow is visible, and its children exceed it's bounds, then presumably I would want the grid to align with the outer bounds of the child item, not the parent. This is not true if I want to animate the height of a grid item as described below.

A little background - I'm working on a pinterest style grid that has animated expanding/collapsing items in it. I've been fighting the stack-grid layout algorithm a bit since I need to coordinate when to update the layout around the animations (since I'm animating height). I refactored some code so that my grid items' height doesn't animate allowing the grid to update immediately with the correct height. At the same time, I am transitioning the height of a grid item child node. This works great when expanding since the grid-item becomes it's final height instantly while the inner child animates to that height. On collapse, however, I have to have overflow: visible on the parent in order to be able to see the transition on the child's height but at the same time, stack-grid wants to immediately measure and transition the grid, but because it uses scrollHeight the calculation is wrong (it accounts for the overflowing child).

I wonder if this is too crazy of a use case to warrant an option - perhaps you could allow the user to define the function that getItemHeight uses to extract the height from the node? If that were the case, I could just pass a function that ignores scrollHeight and just uses clientHeight or offsetHeight.

Thanks!

solomonhawk commented 7 years ago

On another subject (and I can create a separate issue if that's desired), since the container element type can be customized (via component prop) it would make sense to also allow users to define the grid item container element (perhaps via gridComponent or something).

e.g. I want to make the wrapper into a ul and the grid items into li's but right now the <span> that wraps each grid item is hard-coded and passing component="ul" results in invalid markup where span are the directly children of the list instead of li elements.

wadackel commented 6 years ago

Hi @solomonhawk Thank you for Issues :smiley:

perhaps you could allow the user to define the function that getItemHeight uses to extract the height from the node?

It seems to be a little special use case, but I agree with the option addition :+1: If possible send us a PR and it will be very helpful :pray:


As you can see, the gridComponent option is essential for adjusting markup. I would like to respond immediately :muscle:

wadackel commented 6 years ago

@solomonhawk Released a version including itemComponent props as v0.7.0 :tada:.

solomonhawk commented 6 years ago

Awesome! Thanks @tsuyoshiwada. I'll try to find time to prepare a PR for the getItemHeight option.