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

feat: masonry #138

Merged
merged 5 commits into from
Aug 25, 2023
Merged

feat: masonry #138

merged 5 commits into from
Aug 25, 2023

Conversation

gkartalis
Copy link
Member

@gkartalis gkartalis commented Aug 8, 2023

This PR resolves PHIRE-3

Description

Masonry support for tabs.

In order to make masonry work properly without flashes and re-rendering issues had to do two PRs to our dependencies.

One to react-native-collapsible-tab-view and one on @shopify/flash-list

The current PR points to both of my forks just to make sure that it actually solves our issue and it does!

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

Follow ups:

  • update react native collapsible tab view pr to be ready to get merged
  • wait for flash-list pr to get merged or use the forked repo as we do in this PR (maybe move it under Artsy instead of me for everyone to have access to edit?)

Next steps:

How do you think we should proceed with this? It's ready but we are depending on both libraries on merging the PRs.

⚠️ FYI: I have two branches in my flash list fork. One is for the actual PR itself and the other branch (the one that I have on this package.json for flashlist) is the one that also includes the dist folder of the library since it throws errors because dist was in gitignore.

Not sure how to unblock this to publish a canary 🤔 Ci fails with error:
info There appears to be trouble with your network connection. Retrying...

But people in stackoverflow say that this might happen for either no network connection on ci or even having a huge package (first thing that comes to mind is that this is due to me including the dist folder)

Happy to hear your thoughts.

How to run - test

  • Link palette locally with yarn local-palette-dev on this branch gkartalis/masonry
  • Download eigen in this branch gkartalis/masonry and also run yarn local-palette-dev

Demo from eigen

It is missing the filter sticky header but can be really easily added 🔥

Simulator.Screen.Recording.-.iPhone.14.Plus.-.2023-08-08.at.19.11.38.mp4
📦 Published PR as canary version: 13.0.0--canary.138.614.0

✨ Test out this PR locally via:

npm install @artsy/[email protected]
# or 
yarn add @artsy/[email protected]

@gkartalis gkartalis self-assigned this Aug 8, 2023
@artsy-peril artsy-peril bot added the Version: Minor A deploy for new features label Aug 8, 2023
@artsy-peril artsy-peril bot added the Jira Synced Indicates that Peril has connected this PR to Jira label Aug 8, 2023
@damassi
Copy link
Member

damassi commented Aug 8, 2023

That network connection issue is often an ephemeral thing... give it a sec!

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

Amazing 👌

@gkartalis
Copy link
Member Author

PR Got merged, making sure that everything works after I publish the canary. In theory we might not need the flashlist pr to get merged (tested it out without the flashlist patch on the collapsible tab view lib and was working great!)

@gkartalis
Copy link
Member Author

Published canary version: 12.0.2--canary.138.576.0

@gkartalis
Copy link
Member Author

gkartalis commented Aug 10, 2023

UPDATE: we don't need the flashlist PR for this to work, we only need it for the type to be happy and to remove the expect ts error from the collapsible tab library!

@gkartalis gkartalis changed the title feat(wip): masonry feat: masonry Aug 10, 2023
@gkartalis gkartalis added major and removed Version: Minor A deploy for new features labels Aug 10, 2023
@gkartalis gkartalis added canary and removed canary labels Aug 25, 2023
@gkartalis gkartalis merged commit c24a809 into main Aug 25, 2023
@gkartalis gkartalis deleted the gkartalis/masonry branch August 25, 2023 08:55
@artsyit
Copy link
Collaborator

artsyit commented Aug 25, 2023

🚀 PR was released in v13.0.0 🚀

@artsyit artsyit added the released This issue/pull request has been released. label Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
canary Jira Synced Indicates that Peril has connected this PR to Jira major released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants