-
Notifications
You must be signed in to change notification settings - Fork 573
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-253): create suggested artworks screen #9233
feat(ONYX-253): create suggested artworks screen #9233
Conversation
@dariakoko - can you 🙏 update the PR with a full description? We shouldn't depend on external JIRA ticket links to provide relevant context in a PR. Folks reviewing -- Feeling somewhat 😓 having to ping PR after PR about this, #dev channel, etc. It should be flagged and discussed amongst product teams and made an immutable habit. |
Sure. I have just marked it as ready for review. Next time will update the details first and then change the status |
ty! didn't realize it wasn't yet ready to review, saw PR comments and so on. All good. And please spread the word 😄 |
…ng to the artwork page)
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.
cool! Left some questions, let me know if I should add more details!
src/app/Scenes/Artwork/Components/BrowseSimilarWorksModal/BrowseSimilarWorksModalContent.tsx
Outdated
Show resolved
Hide resolved
src/app/Scenes/Artwork/Components/BrowseSimilarWorksModal/BrowseSimilarWorksModalContent.tsx
Outdated
Show resolved
Hide resolved
src/app/Scenes/Artwork/Components/BrowseSimilarWorksModal/BrowseSimilarWorksModalContent.tsx
Outdated
Show resolved
Hide resolved
aggregations, | ||
attributes, | ||
entity, | ||
onClosePress: () => {}, |
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.
question: is there a reason why we pass empty callbacks here?
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.
Not at all, I just wanted to reuse the types we use for the alert creation flow. After refactoring it doesn't make sense any more - removed, added an interface with only aggregations
, attributes
, entity
to the file
Hey @dariakoko, I see you're still pushing to the PR. Is it ready for review? if not would you mind changing it to a draft PR or adding [WIP] to the title. |
@MounirDhahri Ready for review, pushed the latest changes just now |
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.
All good, I left 1 code related suggestion and 1 thing I think we should fix before merging - hide the button if there are no artworks to suggest
well done! 👍
|
||
const similarArtworksQuery = graphql` | ||
query BrowseSimilarWorksContentQuery($input: FilterArtworksInput, $first: Int) { | ||
artworksConnection(first: $first, input: $input) { |
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: there is a a new query you could use instead, which doesn't require attributes (all logic is on metaphysics side):
artwork(id: "banksy-di-faced-tenner-232") {
savedSearch {
suggestedArtworksConnection(first: 10) {
edges {
node {
title
}
}
}
}
}
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.
Good to know, would be nice to use it
I tried it, but it returns an empty array of nodes, so I'd stick only with the totalCount in this commit
We could look into this together if you're interested
src/app/Scenes/Artwork/Components/BrowseSimilarWorks/BrowseSimilarWorksContent.tsx
Outdated
Show resolved
Hide resolved
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.
Really nice PR! great work Daria
src/app/Scenes/Artwork/Components/BrowseSimilarWorks/BrowseSimilarWorks.tsx
Show resolved
Hide resolved
src/app/Scenes/Artwork/Components/BrowseSimilarWorks/BrowseSimilarWorks.tsx
Show resolved
Hide resolved
) | ||
|
||
const similarWorksFragment = graphql` | ||
fragment BrowseSimilarWorks_artwork on Artwork { |
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.
issue: using the same fields here as in
fragment CreateArtworkAlertModal_artwork on Artwork { |
src/app/Scenes/Artwork/Components/BrowseSimilarWorks/BrowseSimilarWorksContent.tsx
Show resolved
Hide resolved
src/app/Scenes/Artwork/Components/BrowseSimilarWorks/BrowseSimilarWorksContent.tsx
Outdated
Show resolved
Hide resolved
src/app/Scenes/Artwork/Components/BrowseSimilarWorks/BrowseSimilarWorksContent.tsx
Outdated
Show resolved
Hide resolved
src/app/Scenes/Artwork/Components/BrowseSimilarWorks/BrowseSimilarWorksContent.tsx
Outdated
Show resolved
Hide resolved
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.
👍
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.
LGTM. Just a small suggestion
c53221b
This PR resolves ONYX-253
Description
Create a modal displaying similar works
We should display only 10 artworks
By pressing on the artwork navigate to the artwork screen
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.