Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added MasonryFlashList which adds support for rendering masonry layouts #587

Merged
merged 49 commits into from
Sep 21, 2022

Conversation

naqvitalha
Copy link
Collaborator

@naqvitalha naqvitalha commented Sep 7, 2022

Description

resolves #455
resolves #575
resolves #589

This PR adds support for Masonry layout rendering also referred to as Staggered Grid View layout.

It's implemented using core FlashList and I didn't plan to make it part of this repo (initially) because it's technically like a derivative. I changed my mind because at the end of the day masonry is a layout and how it's implemented shouldn't matter. Ideally, anything that can be built using FlashList can be kept out. Keeping it lean is a win and should keep the core quality high.

I took the liberty to fix two additional bugs I found while building this otherwise there's no change in core. I've added comments to those changes.

Why use this approach?

It can be implemented using nested lists without any drawbacks and also doesn't require a new layout algorithm on the native side. It's incredibly fast as well.

Performance

It's incredibly well performing. Compared to 2x2 non masonry this is just 2% slower which may very well be a measurement problem.

Known issue

There's a prop type warning being thrown by RLV which I'll address when we upgrade RLV the next time.

Reviewers’ hat-rack 🎩

  • Open fixture and try the masonry sample

Screenshots or videos (if needed)

Checklist

@naqvitalha naqvitalha changed the title Masonry Added MasonryFlashList which adds support for rendering masonry layouts Sep 7, 2022
@Nantris
Copy link

Nantris commented Sep 20, 2022

How are the odds this will be merged?

@naqvitalha
Copy link
Collaborator Author

@fortmarek this is good to go IMO. Thoughts?

<img src="https://user-images.githubusercontent.com/7811728/188055598-41f5c961-0dd0-4bb9-bc6e-22d78596a036.png" height="500"/>
</div>

To get started, import `MasonryFlashList` from `@shopify/flash-list` and use it just like you would use `FlashList`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth here just putting a code snippet? I know it will be basically identical with flash-list but imho still a good practice.

documentation/docs/guides/masonry-layout.md Outdated Show resolved Hide resolved
documentation/docs/guides/masonry-layout.md Outdated Show resolved Hide resolved
documentation/docs/guides/masonry-layout.md Outdated Show resolved Hide resolved
documentation/docs/guides/masonry-layout.md Outdated Show resolved Hide resolved
documentation/docs/guides/masonry-layout.md Outdated Show resolved Hide resolved
fixture/src/Masonry.tsx Show resolved Hide resolved
src/MasonryFlashList.tsx Show resolved Hide resolved
emptyScrollEvent.nativeEvent.doNotPropagate = true;
onScrollProxy?.(emptyScrollEvent);
emptyScrollEvent.nativeEvent.doNotPropagate = false;
}, 32);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32 is two frames which is the minimum I need.

let's also add information about this. The amount of time for two frames also depends on the screen refresh rate but I'm ok with treating 60 Hz as the default.

src/MasonryFlashList.tsx Outdated Show resolved Hide resolved
@fortmarek
Copy link
Contributor

I have only a couple of minor comments, mostly about the documentation.

Additionally, as mentioned here, for now I'd mark this component as unstable or alpha before we are more confident in the stability of this feature.

@naqvitalha
Copy link
Collaborator Author

@fortmarek Addressed the comments! Thanks for the review again.
Regarding stability, it's very stable and ready for prime time. I don't see a need for marking it alpha or beta. The warning that you see is a proptype warning and doesn't impact stability in any way.

Copy link
Contributor

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's goo 🚢 🚢

@naqvitalha naqvitalha merged commit b58b5a6 into main Sep 21, 2022
@naqvitalha naqvitalha deleted the masonry branch September 21, 2022 17:03
@farhanhaider1
Copy link

This looks really good 👏
I have a couple of comments and questions but overall, I love the idea of having multiple FlashLists ❤️
Additionally, I got this warning when running the Masonry example: image

This requires a proptype fix in RLV. I'll get to it later

any update on this?
i am still getting this warning.

@naqvitalha
Copy link
Collaborator Author

It should be fixed in v1.4.0

@Nantris
Copy link

Nantris commented Jan 22, 2023

Does overrideItemLayout not work with Masonry layout? If so, is that documented anywhere? We can't get overrideItemLayout working at all.

@fortmarek
Copy link
Contributor

Does overrideItemLayout not work with Masonry layout? If so, is that documented anywhere? We can't get overrideItemLayout working at all.

Can you flag an issue for this, please? 🙏 I'm personally not aware that Masonry layout doesn't support overrideItemLayout.

@naqvitalha
Copy link
Collaborator Author

Yes, it's not possible to change spans in masonry. We'd appreciate a PR updating the docs!

@Nantris
Copy link

Nantris commented Jan 24, 2023

That's terribly upsetting to hear! We just did hours of work to implement MasonryFlashList. Isn't there some way to allow overrideItemLayout - even if it's imperfect? I'd much rather that to having to roll back hours of work - considering FlashList appears to be the future and I'm confident a good/proper solution will exist in the future even if it doesn't today.

Any chance to simply patch masonry to accept overrideItemLayout? Or some alternative approach? I've searched all afternoon without luck.

@Nantris
Copy link

Nantris commented Jan 24, 2023

@naqvitalha if this is treated as a docs-only issue - at minimum these pages need updating:

There are a lot of places that will need updating to specify things that don't work in masonry mode it seems.

Since this is likely to be a high demand feature in the long run, it seems better to focus on adding it in the near to medium-term future - but that's easy for me to say as I'm not a maintainer.

@coofzilla
Copy link

any update on overrideItemLayout? I just tried implementing to adjust the spans but I guess its still not supported?

@Nantris
Copy link

Nantris commented Jul 26, 2023

The docs were never updated as far as I know. It sure would be nice if this was supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants