-
Notifications
You must be signed in to change notification settings - Fork 25
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
Adds the subscription and purchase pages #963
base: main
Are you sure you want to change the base?
Adds the subscription and purchase pages #963
Conversation
Initial commit for adding the the subscription and purchase pages.
👷 Deploy request for mirlo pending review.Visit the deploys page to approve it
|
Moved the Confetti component to the common directory since we call it in two places: Merch and Tip CheckoutComplete components. Maybe at a later point, we can deduplicate and have one CheckoutComplete component for both. Also nested the new route under `:artistId` instead of it being separate.
Added a query for tips and (have tried to) use it in Tip/CheckoutComplete. Will need more work to make it conform to the types and overall conventions.
Use the `artistId` string instead of the `artist.id` value which is a number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR with a first pass at writing the query code and addressing the comments from the first round of review 🙌
Fixed reference to avatar sizes as there would only be one avatar picture.
const { artistId, tipId } = useParams(); | ||
const { data: artist, isLoading: isLoadingArtist } = useQuery( | ||
queryArtist({ artistSlug: artistId ?? "" }) | ||
); | ||
const { data: tip, isLoading: isLoadingTip } = useQuery( | ||
queryTip({ tipId: tipId }) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the lines:
const { data: artist, isLoading: isLoadingArtist } = useQuery(
queryArtist({ artistSlug: artistId ?? "" })
);
should queryArtist({ artistSlug: artistId ?? "" })
be changed to queryArtist({ artistId: artistId ?? "" })
?
The lines
const { data: tip, isLoading: isLoadingTip } = useQuery(
queryTip({ tipId: tipId })
);
have the const { data: tip, isLoading: isLoadingTip }
greyed out as they are unused. I wonder if we should modify it to
const { data: tip } = useQuery(
queryTip({ tipId: tipId })
);
instead? Also the queryTip({ tipId: tipId })
is failing on a type error:
Type 'string | undefined' is not assignable to type 'string'.
not sure how to modify the caller without painting myself into a corner. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should queryArtist({ artistSlug: artistId ?? "" }) be changed to queryArtist({ artistId: artistId ?? "" })?
Using artistSlug
should be fine. Either way it'll be a string, and the query uses artistSlug
, and the API can handle either one.
have the const { data: tip, isLoading: isLoadingTip } greyed out as they are unused. I wonder if we should modify it to [etc]
That's your call. If you don't want to show a loading indicator while this loads, then you don't need to.
Also the queryTip({ tipId: tipId }) is failing on a type error:
Type 'string | undefined' is not assignable to type 'string'.
The two options here are to let the queryTip function accept an undefined value, or to nest the component in a sub component for which you guarantee that the tip exists. Psuedocode:
const { tipId } = useParams();
if (!tipId) { return null }
return <TipComponent id={tipId} />
and then the tipComponent:
const TipComponent: React.FC<{tipId: string}> = { tipId } => {
// other process
}
Fixes #257