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

fix: problematic android infinite scroll grids #9013

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Conversation

gkartalis
Copy link
Member

@gkartalis gkartalis commented Jul 19, 2023

This PR resolves PHIRE-100

Description

There was a report that after migrating to collapsible-tabs from stickyTabs the infinite scrolling stopped working on Android.

The issue was that we use infiniteScrollArtworkGrid in these surfaces that uses a different implementation (scrollview) for iOS and (ParentAwareScrollView) on Android.

Wrapping it to another scrollview broke the functionality, this PR fixes this behavior on the following ArtworkGrids:

  • GeneArtworks
  • TagArtworks
  • MyCollectionArtworks (didn't add a video for that as I don't have a lot of mycollection artworks to test out the functionality)
  • PartnerArtworks

Let me know if you have any concerns, will add a test case for that on the sprintly mobile QA tomorrow to test it out thouroughly.

Videos

  • Gene
Screen.Recording.2023-07-20.at.10.47.48.mov
  • Partner
Screen.Recording.2023-07-20.at.10.46.52.mov

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

iOS user-facing changes

Android user-facing changes

  • fixed problematic android infinite scroll grids - gkartalis

Dev changes

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

@gkartalis gkartalis self-assigned this Jul 19, 2023
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Jul 19, 2023

This PR contains the following changes:

  • Android user-facing changes (fixed problematic android infinite scroll grids - gkartalis)

Generated by 🚫 dangerJS against f76ec4c

@gkartalis gkartalis marked this pull request as ready for review July 20, 2023 08:59
Copy link
Contributor

@araujobarret araujobarret left a comment

Choose a reason for hiding this comment

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

Thanks for fixing those 💚
I wonder if we have any other infinite scrolls layout impacted by the bug that we might be missing but we can address them in follow-up PRs

Copy link
Member

@MrSltun MrSltun left a comment

Choose a reason for hiding this comment

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

Thank you for this fix George!! 🚀🚀🚀

@gkartalis
Copy link
Member Author

@araujobarret This is how I searched for all affected other surfaces:

The problem is when you have a <Tabs.ScrollView> that has inside a <InfiniteScrollArtworksGrid.

Searched for references of <InfiniteScrollArtworksGrid inside eigen and looked into the tree for the parents to see if it is wrapped on <Tabs.ScrollView>

Feel free to also check in case I missed anything

@gkartalis
Copy link
Member Author

Actually I did miss some while rechecking @araujobarret :P following up with a commit

@gkartalis
Copy link
Member Author

@araujobarret I didn't miss any in the end, they were some of them that had the Show More instead of an infinitescroll that is working properly (one example is artist series artwork grid)

@araujobarret
Copy link
Contributor

Awesome, thanks for the extra care!

@gkartalis gkartalis merged commit e8420c3 into main Jul 20, 2023
1 check passed
@gkartalis gkartalis deleted the gkartalis/PHIRE-100 branch July 20, 2023 10:36
@MounirDhahri
Copy link
Member

Thanks George 🙏

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.

5 participants