paulcollett / react-masonry-css

React Masonry layout component powered by CSS, dependancy free
https://paulcollett.github.io/react-masonry-css/demo/
MIT License
961 stars 66 forks source link

Condionally rendered children cause wrong column placement #59

Closed i2xzy closed 3 years ago

i2xzy commented 4 years ago

In React falsy children are not rendered so it's possible to conditionally render children using logical operators or a ternary in the JSX like so:

<MyMasonryComponent>
  <FirstChild />
  {shouldShow && <SecondChild />}
  <AnotherChild />
</MyMasonryComponent>

however when using react-masonry-css it seems that the falsy child is treated as a valid element when calculating the columns so that in such a situation the 2 truthy children are put in 1 column and another empty column is added to the DOM instead of each child being in a separate column.

I was able to stop that from happening in my case by filtering the children passing into my masonry component but something like should probably be done inside this package:

<Masonry
  breakpointCols={2}
  className="masonry-grid"
  columnClassName="masonry-grid_column"
>
  {_.filter(props.children)}
</Masonry>
paulcollett commented 4 years ago

This has come up before indirectly, and due to it being a breaking, and unrelated to the change at the time, wasn't pushed forward with due to the considerations around how to communicate the API change.

Now that you've called it out, i think we should put a solution back into motion. And, it shouldn't be too hard to rollout as i doubt it would effect the majority of existing users - especially because our handling is unexpected to how react natively handles such cases

CapsE commented 4 years ago

I'm currently facing the same problem. I would be suprised if somebody out there used this behaviour on purpose. It seems very much like a bug to me. Why calculate space for something that's not there anymore? I'll be able to use the filter workaround for now though so thanks for that @i2xzy :)

prasid444 commented 4 years ago

Any updates in this issue?

paulcollett commented 4 years ago

An upgrade path has been planned, but I can’t promise any ETA. For now, please try the above workaround, potentially wrapping it in its own component like (untested):

const SafeMasonry = ({ children, ...props}) => <Masonry {...props}>{[].concat(children).filter(Boolean)}</Masonry>

this will also work with the final fix

paulcollett commented 3 years ago

fixed in https://github.com/paulcollett/react-masonry-css/commit/53d5c9b975f8dbf9b3834e5ffbac638d8be84f80