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

chore: spike on skeletons #10545

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

MounirDhahri
Copy link
Member

@MounirDhahri MounirDhahri commented Aug 1, 2024

This PR resolves []

Description

This is an early spike on how we can get the right skeletons ahead regardless of the rails order.
The idea here is simple, get the available sections and the order without data -> Use that for drawing skeletons while the data is coming from the nested query.
This adds of course an extra query to all home screen fetches - it is however a cheap one since does only one trip to MP and comes back without talking to any other services.

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

Dev changes

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

@artsy-peril
Copy link
Contributor

artsy-peril bot commented Aug 1, 2024

Warnings
⚠️ Please assign someone to merge this PR, and optionally include people who should review.

Generated by 🚫 dangerJS against 812eec2

@ArtsyOpenSource
Copy link
Contributor

Warnings
⚠️

An error occurred while validating your changelog, please make sure you provided a valid changelog.

Generated by 🚫 dangerJS against 812eec2

@joeyAghion
Copy link
Contributor

I could see this working for legacy sections which almost all appear. For the new experience, I worry that Metaphysics may have to load all of the related data in order to determine the visible, personalized sections for a given user. Requesting the sections first and the data next might end up doing twice the work. We can wait and see if that is indeed an issue though.

@MounirDhahri
Copy link
Member Author

When I tried this in Insomnia, I was actually able to get the available sections without MP needing to query other services and that resulted in a faster initial query. It doesn't however solve the issue of having no data available
Screenshot 2024-08-02 at 16 14 22

Screenshot 2024-08-02 at 16 14 37

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