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(CollectionsByCategory): collections ordering #11040

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

araujobarret
Copy link
Contributor

@araujobarret araujobarret commented Oct 30, 2024

Description

Passes some sorting CURATED to MP to receive the collections in the right order.

Extra: adjust Discover Something new event tracking to use tappedCollectionGroup

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2024-10-30.at.14.44.39.mp4

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

  • feat: collections by category sorting

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.

@araujobarret araujobarret requested a review from a team October 30, 2024 13:46
@araujobarret araujobarret self-assigned this Oct 30, 2024
egdbear
egdbear previously approved these changes Oct 30, 2024
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Oct 30, 2024

This PR contains the following changes:

  • Cross-platform user-facing changes (feat: collections by category sorting - araujobarret)

Generated by 🚫 dangerJS against 477a92c

collectionID: card.entityID,
contextModule: section.contextModule as ContextModule,
index,
})
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this now using the wrong analytics event? Not all section instances of type HomeViewSectionCardChips are guaranteed to represent a grouping of Marketing Collections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Louis from @artsy/data-platform asked to change it to the tappedCollectionGroup event, gonna pass the ⚽ to him

Copy link
Member

@dblandin dblandin Oct 30, 2024

Choose a reason for hiding this comment

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

If this PR were slimmed down to just the CURATED sort change, I think we could merge it right away but I want to chat with Louis about the event before changing the analytics here.

Choose a reason for hiding this comment

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

The DiscoverSomethingNew rail seems like a rail of collections, rather than cards!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dblandin reverted the tracking changes here e392b74

Copy link
Member

@dblandin dblandin left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@araujobarret araujobarret force-pushed the araujobarret/feat/collection-by-categories-ordering branch from e392b74 to 477a92c Compare October 30, 2024 16:13
@araujobarret araujobarret added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Oct 30, 2024
@artsy-peril artsy-peril bot merged commit b1e8d3d into main Oct 30, 2024
7 checks passed
@artsy-peril artsy-peril bot deleted the araujobarret/feat/collection-by-categories-ordering branch October 30, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Art Advisor Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants