-
Notifications
You must be signed in to change notification settings - Fork 82
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: Add MANA approval to the buy items slots flow #1707
feat: Add MANA approval to the buy items slots flow #1707
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/decentraland/builder/7vcUSYSEYEM8ELVgyzuJ1ubqkxtq |
Pull Request Test Coverage Report for Build 1658645763
💛 - Coveralls |
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.
A few silly things
@@ -16,6 +16,12 @@ export default class BuyItemSlotsModal extends React.PureComponent<Props, State> | |||
selectedTierId: undefined | |||
} | |||
|
|||
onCloseModal = (): void => { |
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.
To follow conventions, I think this should be called handleOnCloseModal
right?
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.
Changed it!
onTierSelected: () => dispatch(clearTiersError()), | ||
onBeforeClose: () => dispatch(clearTiersError()), |
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.
This might be coincidental, but given that we have two methods with different names poiting to the same dispatched action, what if we just have onClearTiersError()
and call that instead?
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 definitely though about it, but from the perspective of the component as an isolated chunk of code, there's nothing to explicitly clear the errors, so it might be a bit confusing.
const MANA_SYMBOL = '⏣' | ||
|
||
export function addSymbol(num: string | number) { | ||
return num > 0 ? `${MANA_SYMBOL} ${num.toString()}` : '' | ||
} | ||
|
||
export function buildManaAuthorization(address: string, chainId: ChainId, contractName: ContractName): Authorization { |
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 can test this method?
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.
Oh, I forgot to add tests to it! Added them.
…ub.com:decentraland/builder into feat/add-mana-approve-modal-to-buy-items-slots
This PR does the following: