feat: Build item slots #1664
feat: Build item slots #1664
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/decentraland/builder/DZGEUs6BSUgGwcgkjL45JHdYDq2j |
Pull Request Test Coverage Report for Build 1463278570
💛 - Coveralls |
| const { wallet, collection } = this.props | ||
| return collection !== null && canSeeCollection(collection, wallet.address) | ||
| const { wallet, collection, thirdParty } = this.props | ||
| return collection && thirdParty && isUserManagerOfThirdParty(wallet.address, thirdParty) |
There was a problem hiding this comment.
This is enough to verify that the user has access to the collection. What do you think about this @nicosantangelo ?
There was a problem hiding this comment.
Yup I think it's ok!
There was a problem hiding this comment.
Things I've noticed using the UI:
- The loading indicator is too close to the text, which makes it a bit jarring when the data eventually appears
- The "you don't have enough MANA notice has more padding at the bottom that at the top which makes it a bit weird. Also, there seem to be two spaces between the MANA symbol and
MANA
- The same described above happens with the error message. Furthermore, picking a new slot does not clean the error and there's no
Xto do it either which can be confusing.
- MINOR or NOT necessary: Maybe we can align the MANA symbols vertically?:
| import { Button, ModalDescription, ModalHeader, CheckboxProps, Radio, Mana, Loader, Message } from 'decentraland-ui' | ||
| import { T, t } from 'decentraland-dapps/dist/modules/translation/utils' | ||
| import { Modal, NetworkButton } from 'decentraland-dapps/dist/containers' | ||
| import { env } from 'decentraland-commons' | ||
| import { Props } from './BuyItemSlotsModal.types' | ||
| import { Network } from '@dcl/schemas' | ||
| import { ThirdPartyItemTier } from 'modules/tiers/types' | ||
| import styles from './BuyItemSlotsModal.module.css' |
There was a problem hiding this comment.
Can we order these? At lest to leave the remote imports at the top
| const sortTiers = (a: ThirdPartyItemTier, b: ThirdPartyItemTier) => { | ||
| if (a.value < b.value) { | ||
| return -1 | ||
| } else if (a.value > b.value) { | ||
| return 1 | ||
| } | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Shouldn't this live in modules/tiers/utils?
I see that we're doing the same with sortLandPoolLast so maybe it's not necessary
There was a problem hiding this comment.
Moved it to modules/tiers/utils. I left it there because it was the only place that it was going to be used.
I also added tests for it.
| onFetchThirdPartyItemSlots: typeof fetchThirdPartyItemTiersRequest | ||
| manaBalance: number | ||
| tiers: ThirdPartyItemTier[] | ||
| metadata: { thirdParty: ThirdParty } |
There was a problem hiding this comment.
Just for consistency sake, could we extract this to it's own type? on the same file. Like this:
export type Props = ModalProps & {
metadata: ShareModalMetadata
(...)
}
export type ShareModalMetadata = {
type: ShareModalType
id: string
}And if you want to keep the callbacks (on(...)) at the end, that's peachy too
There was a problem hiding this comment.
Moved the callbacks at the end and created the Metadata type!
| const collection = this.props.collection! | ||
| const slots = 0 | ||
| const slots = thirdParty ? new BN(thirdParty.maxItems).sub(new BN(thirdParty.totalItems)) : new BN(0) |
There was a problem hiding this comment.
could this be a candidate for a utils method? like getCurrentSlots(thirdParty)
There was a problem hiding this comment.
Moved it and tested it.
| @@ -14,6 +15,7 @@ import CollectionPublishButton from './CollectionPublishButton' | |||
| import { Props } from './ThirdPartyCollectionDetailPage.types' | |||
|
|
|||
| import './ThirdPartyCollectionDetailPage.css' | |||
| import { isUserManagerOfThirdParty } from 'modules/thirdParty/utils' | |||
There was a problem hiding this comment.
If you don't mind, Could we move this a few lines below the other module imports?
src/modules/tiers/reducer.ts
Outdated
| error: null | ||
| } | ||
|
|
||
| type ThirdPartyReducerAction = |
There was a problem hiding this comment.
TiersReducerAction !
src/modules/tiers/reducer.ts
Outdated
|
|
||
| const INITIAL_STATE: TiersState = { | ||
| data: { | ||
| thirdParty: [] |
There was a problem hiding this comment.
Just making sure, this property is here so we can, in the future, add tiers to other parts of the state right?
I ask mostly because it's called thirdParty but not thirdPartyItems when the type is ThirdPartyItemTier
There was a problem hiding this comment.
It is set like that for the purpose you've mentioned.
You're right, it should be called thirdPartyItems as it defines the tiers of the items in a more proper way.
src/modules/tiers/selectors.ts
Outdated
| import { FETCH_THIRD_PARTY_ITEM_TIERS_REQUEST, BUY_THIRD_PARTY_ITEM_TIERS_SUCCESS, BUY_THIRD_PARTY_ITEM_TIERS_REQUEST } from './actions' | ||
| import { ThirdPartyItemTier, TiersState } from './types' | ||
|
|
||
| function getState(state: RootState): TiersState { |
There was a problem hiding this comment.
I don't really mind this, but if you want, we could change this file to use constants
export const getState (...)so it looks like all the other selectors in the codebase/other dapps.
Following that path, we can keep consistent and add the four selectors we have in all files:
export const getState = (state: RootState) => state.item
export const getData = (state: RootState) => getState(state).data
export const getLoading = (state: RootState) => getState(state).loading
export const getError = (state: RootState) => getState(state).errorwhich'd allow for not having to do .data below too
There was a problem hiding this comment.
We're using lambdas in all of our other selectors, so I don't mind at all! Changed.
src/modules/wallet/selectors.ts
Outdated
| export function getManaBalance(state: RootState): number { | ||
| const wallet = getWallet(state) | ||
|
|
||
| return wallet ? wallet.networks.MATIC.mana : 0 | ||
| } |
There was a problem hiding this comment.
Careful here, this method is misleading. There's no one mana balance, it always should depend on the network. Furthermore, decentraland dapps already has a getNetworks which can be accessed by network to get the mana balance (deprecating this selector)
I think we can just use getNetworks where this selector is used and pass Netowrk.MATIC if necessary
There was a problem hiding this comment.
I took this section of code from here.
Can't we have something like getManaBalanceForNetwork(state:RootState, network: Network)? Because as far as I'm aware (I might be mistaken), the contract will live in the MATIC network. This also opens the window for the the question "should we should this modal at all if we're on another network that's not MATIC?" because we could disable the "Buy slots" button in that case.
There was a problem hiding this comment.
Created the getManaBalanceForNetwork(state:RootState, network: Network) method!
tsconfig.json
Outdated
| "noUnusedLocals": false, | ||
| "isolatedModules": true, | ||
| "esModuleInterop": true, | ||
| "strictNullChecks": true, | ||
| "noImplicitReturns": true, | ||
| "resolveJsonModule": true, | ||
| "noUnusedParameters": true, | ||
| "noUnusedParameters": false, |
There was a problem hiding this comment.
After seeing this I've opened BuyItemSlotsModal.container.ts and saw that Dispatch is imported and not being used :(
There was a problem hiding this comment.
Ohh, I removed it to make the development a bit faster but I forgot about re-adding it again.
| @@ -0,0 +1,8 @@ | |||
| import { Network } from '@dcl/schemas' | |||
| import { RootState } from 'modules/common/types' | |||
| import { getNetworks } from 'decentraland-dapps/dist/modules/wallet/selectors' | |||
There was a problem hiding this comment.
Can we move this a line above 👉👈
tsconfig.json
Outdated
| @@ -11,7 +11,7 @@ | |||
| "skipLibCheck": true, | |||
| "noImplicitAny": true, | |||
| "noImplicitThis": true, | |||
| "noUnusedLocals": true, | |||
| "noUnusedLocals": false, | |||
There was a problem hiding this comment.
Changed it to the value it had before.
| render() { | ||
| const { onClose, isFetchingTiers, name, isBuyingItemSlots, tiers, error } = this.props | ||
| const { selectedTierId } = this.state | ||
| console.log('Starting this shit') |
There was a problem hiding this comment.
Ups, removed it!
This PR adds the
BuyItemSlotsModalwith the required module to buy slots and attaches it to theThirdPartyCollectionDetailPage.Closes #1573