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(DIA-1017): add bottom sheet to infinite discovery #11318

Merged

Conversation

araujobarret
Copy link
Contributor

@araujobarret araujobarret commented Dec 19, 2024

This PR resolves DIA-1017

Description

This PR adds the bottom sheet element to the infinite discovery screen. We use preloaded data from the artwork card screen have the data ready if the user wants to swipe to check more details.
For next PRs:

  • handle navigation to external links(we might need dedicated navigation container to keep the Bottom Sheet open)
  • tracking
  • Other works tab

iOS

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2024-12-19.at.23.12.55.mp4

Android

Screen.Recording.2024-12-19.at.23.23.16.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

  • feat: add artwork bottom sheet to infinite discovery(hidden by ff)

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 December 19, 2024 22:27
@araujobarret araujobarret self-assigned this Dec 19, 2024
@@ -46,7 +47,10 @@ export const ArtistAbout: React.FC<Props> = ({ artist }) => {
{!!hasBiography && (
<>
<Spacer y={1} />
<Biography artist={artist} />
<Flex maxWidth={MAX_WIDTH_BIO} px={2}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing changed so far, lifted up some elements to be able to share the Biography element easily

artist: ArtistFollowButton_artist$key
}

export const ArtistFollowButton: FC<ArtistFollowButtonProps> = ({ artist }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separated the personalized data into an individual query.

@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Dec 19, 2024

This PR contains the following changes:

  • Cross-platform user-facing changes (feat: add artwork bottom sheet to infinite discovery(hidden by ff) - araujobarret)

Generated by 🚫 dangerJS against 1abc528

partner: PartnerFollowButton_partner$key
}

export const PartnerFollowButton: FC<PartnerFollowButtonProps> = ({ partner }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a version of the follow button personalized and with an isolated query. Same as we do for ArtistFollows

@@ -3,46 +3,46 @@ import { useMutation, graphql } from "react-relay"
export interface FollowProfileOptions {
id: string
internalID: string
isFollowd: boolean | null
onCompleted?: (isFollowd: boolean) => void
isFollowed: boolean | null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the typo, was even creating ambiguity on the store, if somewhere we updated the isFollowed within the updater it wouldn't reflect to the elements using this one because of the typo.

/**
* @deprecated in favor of PartnerFollowButtonWithSuspense
* @reason moving away personalized data into different queries
*/
@track()
export class PartnerFollowButton extends React.Component<Props, State> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to get rid of this Element at some point, deprecated it to avoid ppl using it

@@ -144,7 +144,7 @@ export const ArtworkHeaderFragmentContainer = createFragmentContainer(ArtworkHea
artists(shallow: true) {
name
}
partner {
partner(shallow: true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since I wanted to use shallow true in the new elements, the shared children started complaining about inconsistencies, so I had to replicate it which is a good improvement in general avoiding extra hits to partner and artist endpoint when the artwork already has all the data

import { setupTestWrapper } from "app/utils/tests/setupTestWrapper"
import { graphql } from "react-relay"

describe("InfiniteDiscoveryBottomSheetFooter", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file decreases the morale of the TDD team, but was failing and I spent a few hours trying to identify why and couldn't figure it out.

image

@araujobarret araujobarret force-pushed the araujobarret/dia-1017/bottom-sheet-infinite-discovery branch from 61d44aa to e80af24 Compare December 20, 2024 10:54
@araujobarret araujobarret force-pushed the araujobarret/dia-1017/bottom-sheet-infinite-discovery branch from e80af24 to 1abc528 Compare January 13, 2025 12:12
@araujobarret araujobarret merged commit 2a9e177 into main Jan 13, 2025
7 checks passed
@araujobarret araujobarret deleted the araujobarret/dia-1017/bottom-sheet-infinite-discovery branch January 13, 2025 12:40
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.

2 participants