feat(ONYX-1294): redesign badges and urgency signals on artwork card #11005
Conversation
src/app/Components/ContextMenu/ContextMenuArtworkPreviewCard.tsx
Outdated
Show resolved
Hide resolved
| @@ -84,7 +84,9 @@ export const saleMessageOrBidInfo = ({ | |||
| } | |||
|
|
|||
| if (collectorSignals?.partnerOffer?.isAvailable) { | |||
| return collectorSignals.partnerOffer.priceWithDiscount?.display | |||
| const salePrice = artwork.saleMessage && `~${artwork.saleMessage}~` | |||
There was a problem hiding this comment.
Are these ~ intentional?
There was a problem hiding this comment.
Yes. saleMessageOrBidInfo returns a string and we display it in a Text component. For the LTO we will need to return discounted price as well as original price and the original price has to have line-through text decoration. In order to not change the returned type I came up with this solution - mark the text that has to have line-through decoration with ~ symbol and then parse the returned string. I'm not sure if this is the best approach, but it seems to work and doesn't require a lot of testing
There was a problem hiding this comment.
question: Should that be handled with a ternary instead of a && though?
I'm wondering if it's possible that artwork.saleMessage could be null or undefined, in which case this could render in an undesirable way with the && trick.
See what I mean…
$ node
// works ok with a valid message
> saleMessage = "valid message"
> salePrice = saleMessage && `~${saleMessage}~`
> `sometext${salePrice}`
'sometext~valid message~'
// could look weird with a null message
> saleMessage = null
> salePrice = saleMessage && `~${saleMessage}~`
> `sometext${salePrice}`
'sometextnull'
// but ternary fixes it
> saleMessage = null
> salePrice = saleMessage ? `~${saleMessage}~` : ''
> `sometext${salePrice}`
'sometext'
(It is possible for an artwork sale message to be null in Gravity, under certain conditions)
There was a problem hiding this comment.
You're right, I missed that 👍
There was a problem hiding this comment.
That's a great solution. To make it more obvious what's happening it could be good to extract the logic into a serialize and a parse function (maybe just declared within the same file). Alternatively two comments could also help to understand when reading the code.
Extracting the logic would be my favorite :)
dariakoko
requested review from
MounirDhahri and
a team
and removed request for
MounirDhahri
| @@ -84,7 +84,9 @@ export const saleMessageOrBidInfo = ({ | |||
| } | |||
|
|
|||
| if (collectorSignals?.partnerOffer?.isAvailable) { | |||
| return collectorSignals.partnerOffer.priceWithDiscount?.display | |||
| const salePrice = artwork.saleMessage && `~${artwork.saleMessage}~` | |||
There was a problem hiding this comment.
question: Should that be handled with a ternary instead of a && though?
I'm wondering if it's possible that artwork.saleMessage could be null or undefined, in which case this could render in an undesirable way with the && trick.
See what I mean…
$ node
// works ok with a valid message
> saleMessage = "valid message"
> salePrice = saleMessage && `~${saleMessage}~`
> `sometext${salePrice}`
'sometext~valid message~'
// could look weird with a null message
> saleMessage = null
> salePrice = saleMessage && `~${saleMessage}~`
> `sometext${salePrice}`
'sometextnull'
// but ternary fixes it
> saleMessage = null
> salePrice = saleMessage ? `~${saleMessage}~` : ''
> `sometext${salePrice}`
'sometext'
(It is possible for an artwork sale message to be null in Gravity, under certain conditions)
dariakoko
deleted the
dariakoko/ONYX-1294/redesign-badges-and-urgency-signals-on-artwork-card
branch
This PR resolves ONYX-1294
Description
Redesign badges and urgency signals on artwork card
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.