cedricdelpoux
feat: distribute children by height
Resolves #117
Before:
After:
Uses a greedy algorithm - this keeps items in a similar order as what was passed in. For the best column distribution you probably need some form of sorting the children like here. You could also use something like MUI's masonry's sequential prop to use the old distribution algorithm.
This is my first time really working with react class components, so I'm not sure if this is a good method. I couldn't really figure out how to get the demo running (had some errors), so I just tested on my own project instead.
Sorry the commit name is misleading, this is not sequential
Hello @yangchristina . Thanks for your help!
I tried you branch but the ResponsiveMasonry demo is broken.
@cedricdelpoux I've fixed it. I was able to run the demo before merging with master, so I've confirmed it works, but I wasn't able to figure out running it after merging, so I can't confirm whether it works or not with the new updates.
Sorry for the delay. @yangchristina can you resolve conflicts? After that I will merge. Thank you
@yangchristina I think it's because your first commit Merge remote-tracking branch 'upstream/master'.
Could you rebase your branch on master and apply only your 3 last commits? Or better squash your 3 last commit in just one?
Thank you
@fostergn thanks for the reminder, I'd forgotten about this. @cedricdelpoux I've rebased it, is this okay? I still can't run it, so would you mind checking if this is still fine?
@yangchristina Sorry I just tried your branch and the ResponsiveMasonry demo is still broken.
The second and third column are in the DOM but empty.
The same demo on master works as expected like here.
you need to run yarn start to run the demo locally
@cedricdelpoux That's what I've been trying but it doesn't work. It just terminates immediately
Before, I got it to run by using node 16, but now even that isn't working.
What node and yarn versions are you using?
@cedricdelpoux Should be fixed now, distributing children by height will only occur when the height of every child is > 0 The problem before was that distributeChildren was running before images had loaded, so they all had a height of 0
Not sure if this is a good solution, since it only reorders things after every image has loaded, so I added a sequential prop to use old behaviour
Running it worked when I ran yarn set version 1.22.21
Sorry @yangchristina, your PR was causing issues. https://github.com/cedricdelpoux/react-responsive-masonry/issues/131
I reverted for now
@yangchristina If you want to re-submit the PR, the fix is rather simple: https://github.com/cedricdelpoux/react-responsive-masonry/issues/131#issuecomment-2516680319
Hi @yangchristina the re-submitted PR is merged.
Would it be possible for you to help update the type definitions here? If you have some experience contributing PRs to DefinitelyTyped:
https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-responsive-masonry
It's actually missing all of these. Maybe I will try it some other time, but I don't have experience contributing to the DT project.
containerTag: PropTypes.string,
itemTag: PropTypes.string,
itemStyle: PropTypes.object,
sequential: PropTypes.bool,
| Name | PropType | Description | Default |
|---|---|---|---|
| containerTag | String | Tag name of the container element | "div" |
| itemTag | String | Tag name of the item element | "div" |
| itemStyle | Object | Style object applied to each item | {} |
| sequential | Boolean | If true, items are placed in the order they are passed | false |
@ADTC I've made a PR for this https://github.com/DefinitelyTyped/DefinitelyTyped/pull/71485
@ADTC you are the awesome one, if it wasn't for you figuring out what the fix for this pr was, I would have gotten quite panicked about causing a regression, so thanks for that!
Thank you @yangchristina!
This right here is the spirit of open source. A community of people volunteering to come together to solve challenges, not for their own benefit, but for the greater good and for the benefit of all.