-
Notifications
You must be signed in to change notification settings - Fork 575
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(ONYX-149): trigger mutations on artist preview bottom sheet #9034
feat(ONYX-149): trigger mutations on artist preview bottom sheet #9034
Conversation
5e4e881
to
b23060a
Compare
b23060a
to
edece5f
Compare
input, | ||
}, | ||
// @ts-expect-error | ||
optimisticResponse: { |
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.
added this for a better ux
variables: { | ||
input, | ||
}, | ||
// @ts-expect-error |
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.
types are old here because of some relay types weirdness, we should at some point fix this everywhere in Eigen
keyExtractor={({ internalID }) => internalID!} | ||
data={filteredUserInterests} | ||
renderItem={({ item }) => { | ||
if (item?.node && item.internalID && item.node.internalID) { |
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.
this is never going to be null because we're checking for it above this call. I added this to make typescript happy
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.
suggestion / non-blocking: very tiny suggestion, I think this can be written as
item.internalID && item.node?.internalID
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.
Great work!
@dariakoko I will merge this to make sure you can continue on the work on showing the privacy icon next to the artist name |
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.
I don't know much about this part of codebase, but looks good to me 👍
|
||
if (!collectedArtists) return <></> | ||
|
||
const filteredUserInterests = userInterests.filter((userInterest) => { | ||
if (userInterest?.internalID && userInterest.node && userInterest.node.internalID) { |
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.
suggestion / non-blocking: very tiny suggestion, I think this can be written as
userInterest?.internalID && userInterest.node?.internalID
keyExtractor={({ internalID }) => internalID!} | ||
data={filteredUserInterests} | ||
renderItem={({ item }) => { | ||
if (item?.node && item.internalID && item.node.internalID) { |
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.
suggestion / non-blocking: very tiny suggestion, I think this can be written as
item.internalID && item.node?.internalID
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.
Thank you!
This PR resolves ONYX-149
Description
This PR adds the missing mutations to the artist preview bottom sheet
Screen.Recording.2023-07-25.at.13.29.30.mov
PS: After recording the video, I realized it's not good UX to delete the artist immediately and added an alert first.
PR Checklist
To the reviewers 👀
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.