feat: nft view page cta module and responsive style updates #55
Conversation
✅ Deploy Preview for zapp-nft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
src/features/view-nft/NFTDetailsCard/ui/UserOfferAction/UserOfferAction.tsx
Outdated
Show resolved
Hide resolved
|
Hey @colbr. I decided to separate all of the actions for this CTA Module. It does feel a little clunky doing it this way but hopefully it means the variations logic is easier to follow. There are still some further UI changes to make, but it would be good to do these when updating the components in zUI. Let me know your thoughts on it :) Thanks! |
src/features/domain-settings/DomainSettingsModal/DomainSettingsModal.tsx
Show resolved
Hide resolved
src/features/view-nft/NFTDetailsCard/ui/CTAContainer/CTAContainer.tsx
Outdated
Show resolved
Hide resolved
| const { isDomainBiddable, isBuyNow, isUserBid, isOwnedByUser } = | ||
| useActionsData(zna); | ||
|
|
||
| const isSingleAction = | ||
| (!isBuyNow && isDomainBiddable) || (isBuyNow && !isDomainBiddable); |
There was a problem hiding this comment.
All the different variations makes this one pretty tricky huh! Perhaps could be nice if, instead of returning booleans from useActionsData, we rename the hook useActions and return the component to render? It would move a bit of the complex logic into the hook, which would make this component a bit more straight forward. This may end up less tidy, but I think it's worth giving a go as sometimes variations get really hard to manage in JSX.
There was a problem hiding this comment.
Yeah nice suggestion thanks mate, like the idea of returning the component to render. I'll give this a go !
| @@ -0,0 +1,63 @@ | |||
| import { FC } from 'react'; | |||
There was a problem hiding this comment.
Noticed that for each of these components there's a OwnerX and a UserX - would it be possible to instead add isOwner is a property and have one component? That would cut down code repetition.
| @@ -13,6 +13,8 @@ import { | |||
| useDomainMetadata, | |||
| usePaymentToken, | |||
| } from '../../../lib/hooks'; | |||
| import { ethers } from 'ethers'; | |||
| import { bigNumberToLocaleString } from '@zero-tech/zapp-utils/formatting/big-number'; | |||
|
|
|||
| export const useActionsData = (zna: string) => { | |||
There was a problem hiding this comment.
This hook is getting a bit tough to read, perhaps we could split the react-query hooks into a sub hook, like
const useActions = (zna) => {
const { data, isLoading } = useActionsData(zna)
...
}
const useActionsData = () => {
// get all the data from react-query
return {
// data: add all the data to an object
// isLoading: isLoading from all the react-query hooks
}
}| @@ -34,13 +36,34 @@ export const useActionsData = (zna: string) => { | |||
| const { sortedBids, highestBid } = sortBidsByAmount(allBids); | |||
There was a problem hiding this comment.
I think it's going to be quite a common pattern to get bids and then sort them - it could be handy to have a hook which returns all of this together, so you could call
const { data, isLoading } = useBids(domainId, { account: account, sortBy: 'amount' })Where the options parameter is optional (useBids(domainId: string, options?: Options)).
| const buyNowPriceUsdConversionString = | ||
| paymentToken && buyNowPrice | ||
| ? `$${formatNumber( | ||
| Number(ethers.utils.formatEther(buyNowPriceString)) * | ||
| Number(paymentToken?.priceInUsd), | ||
| )}` | ||
| : '-'; |
There was a problem hiding this comment.
I've seen this pattern a bit too, wonder if we could make a nice wrapper function for this like
const getTextFallback = (generator: () => string, fallback: string) => {
try {
return generator();
} catch(e) {
return fallback;
}
}
getTextFallback(
() => $${formatNumber(
Number(ethers.utils.formatEther(buyNowPriceString)) *
Number(paymentToken?.priceInUsd),
)},
'-'
)My suggestion might be a little bit convoluted, but I'm sure there's a way to wrap this to make it easier!
| @media only screen and (min-width: 485px) { | ||
| max-width: unset; | ||
| } | ||
|
|
||
| @media only screen and (min-width: 878px) { |
There was a problem hiding this comment.
These viewport-based queries will break when zOS team adds the right-sidekick. Should make a note of this and take them out later.
| export const ActionContainer: FC<ActionContainerProps> = ({ zna }) => { | ||
| const { isDomainBiddable, isBuyNow, isUserBid, isOwnedByUser } = | ||
| useActions(zna); | ||
|
|
||
| const isSingleAction = | ||
| (!isBuyNow && isDomainBiddable) || (isBuyNow && !isDomainBiddable); | ||
|
|
||
| return ( | ||
| <div | ||
| className={cx(styles.ActionContainer, { | ||
| isSingleAction: isSingleAction, | ||
| })} | ||
| > | ||
| {/* Make offer */} | ||
| {(isDomainBiddable || !isBuyNow || isUserBid) && ( | ||
| <UserOfferAction zna={zna} /> | ||
| )} | ||
|
|
||
| {/* Buy now */} | ||
| {isBuyNow && <UserBuyNowAction zna={zna} />} | ||
|
|
||
| {/* View bids / Enable bids now && Set buy now / Edit buy now */} | ||
| {isOwnedByUser && ( | ||
| <> | ||
| <OwnerOfferAction zna={zna} /> | ||
| <OwnerSetBuyNowAction zna={zna} /> | ||
| </> | ||
| )} | ||
|
|
||
| {/* TODO: add styling to handle cancel offer with multiple actions */} | ||
| {/* {!isSingleAction && !isUserBid && <UserCancelOffer zna={zna} />} */} | ||
| </div> | ||
| ); | ||
| }; |
| export const TextValue = ({ | ||
| tokenValue, | ||
| fiatValue, | ||
| isSingleAction, | ||
| }: TextValueProps) => ( | ||
| <div | ||
| className={cx(styles.Values, { | ||
| isSingleAction: isSingleAction, | ||
| })} | ||
| > | ||
| <span className={styles.TokenValue}>{tokenValue}</span> | ||
| <span className={styles.FiatValue}>{fiatValue}</span> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Should be able to use TextDetail from zUI now :)
Associated Notion Card
1. Pull request checklist
2. PR type
Updates to the CTA Module to help improve the NFT View page.
Some examples: