-
Notifications
You must be signed in to change notification settings - Fork 572
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(EMI-2015): Add auction signals to artwork grid and rail components #10669
feat(EMI-2015): Add auction signals to artwork grid and rail components #10669
Conversation
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.
Looks great 🎉
collectorSignals, | ||
auctionSignals, | ||
}: { | ||
artwork: 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.
way nicer to read, thanks for the changes 💟
* @example | ||
* "$1,750 (2 bids)" | ||
*/ | ||
export const saleMessageOrBidInfo = ({ |
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 for now) Is it possible to move this to MP
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.
Maybe we should move it to MP instead of having the client figure it out, but currently we use it with feature flags, so perhaps we could move it after releasing the feature?
<Flex flexDirection="row" alignItems="center" justifyContent="space-between"> | ||
<Text variant="xs" color="black60"> | ||
{!!collectorSignals?.auction?.lotWatcherCount | ||
? `${collectorSignals.auction.lotWatcherCount} Watchers` |
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.
is the lotWatcherCount
always bigger than 2?
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 catch 🙇
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.
just updated it in #10682
} | ||
|
||
const fragment = graphql` | ||
fragment ArtworkAuctionTimer_collectorSignals on CollectorSignals { |
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 wonder what will be the impact of this fragment on the performance of the query - auctions used to not be the fastest thing we have 😄
This PR resolves EMI-2015
Note
The changes in this PR are hidden behind
AREnableAuctionImprovementsSignals
feature flagDescription
This PR adds new signals for auctioned artworks in Artwork Grid items and Artwork Rail cards
Screenshots and videos
Screenshots and Videos
updating-lot-watchers-android.mov
updating-lot-watchers-ios.mov
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.