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

refactor(PHIRE-218): use MasonryFlashList of full screen grids #9332

Merged
merged 13 commits into from
Sep 27, 2023

Conversation

gkartalis
Copy link
Member

@gkartalis gkartalis commented Sep 26, 2023

This PR resolves PHIRE-218

Description

Important

Nothing changed UI wise!

Replacing the following surfaces with the new MasonryFlashList.
Created also a reusable component for FullScreen Masonry grids (there is another one for the tabs).

Refactored surfaces:

  • ArtworkList.tsx
  • ArtworkRecommendations.tsx
  • NewWorksForYou.tsx
  • NewWorksFromGalleriesYouFollow.tsx
  • SimilarToRecentlyViewed.tsx
  • RecentlyViewed.tsx
  • LotsByArtistsYouFollow.tsx

Note

There was also a bonus refactor on the LotsByArtistsYouFollow.tsx surface that refactored the queryRenderer to use relay hooks instead ⭐️

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • refactor full screen artworkgrid surfaces to use new masonry - gkartalis

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

@gkartalis gkartalis self-assigned this Sep 26, 2023
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Sep 26, 2023

This PR contains the following changes:

  • Cross-platform user-facing changes (refactor full screen artworkgrid surfaces to use new masonry - gkartalis)

Generated by 🚫 dangerJS against 7d62286

@gkartalis gkartalis force-pushed the gkartalis/PHIRE-218 branch 2 times, most recently from fe77a00 to 28beee6 Compare September 27, 2023 07:45
@gkartalis gkartalis marked this pull request as ready for review September 27, 2023 13:10
@gkartalis
Copy link
Member Author

A thing that I noticed is that for the LotsByArtistsYouFollow screen we don't have any analytics for pageviews. I will ask in #analytics-help to see if we should add them

height={imgHeight}
navigateToPageableRoute={navigateToPageableRoute}
/>
</Flex>
Copy link
Contributor

Choose a reason for hiding this comment

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

✨ 🧹

Copy link
Contributor

@brainbicycle brainbicycle left a comment

Choose a reason for hiding this comment

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

awesome work, this is looking much nicer!

@gkartalis gkartalis merged commit 901898c into main Sep 27, 2023
@gkartalis gkartalis deleted the gkartalis/PHIRE-218 branch September 27, 2023 16:09
Copy link
Member

@MounirDhahri MounirDhahri left a comment

Choose a reason for hiding this comment

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

sorry just found out I didn't submit my review - lgtm

Comment on lines +47 to +49
<Flex
pl={columnIndex === 0 ? 0 : 1}
pr={NUM_COLUMNS_MASONRY - (columnIndex + 1) === 0 ? 0 : 1}
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: it would be nice to do some explorations on supporting gaps to our Flex component to avoid conditional paddings.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a great point, we can use also https://reactnative.dev/docs/0.71/flexbox#row-gap-column-gap-and-gap when we upgrade to either 0.71 or 0.72

Copy link
Member

Choose a reason for hiding this comment

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

exactly that!

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

Successfully merging this pull request may close these issues.

4 participants