Skip to content
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

fix: incorrect currency displayed in profiledropdown #4135

Merged
merged 8 commits into from
Oct 24, 2022

Conversation

Jarsen136
Copy link
Contributor

Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.

PR type

  • Bugfix
  • Feature
  • Refactoring

What's new?

Before submitting Pull Request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </rmrk/collection/26902bc2f7c20c546a-1FVG7>
  • I've tested PR on mobile and everything seems works
  • I found edge cases
  • I've written some unit tests 🧪

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot

BSX:

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

image

image

image

@Jarsen136 Jarsen136 requested review from a team as code owners October 18, 2022 16:17
@Jarsen136 Jarsen136 requested review from roiLeo and petersopko and removed request for a team October 18, 2022 16:17
@kodabot
Copy link
Collaborator

kodabot commented Oct 18, 2022

SUCCESS @Jarsen136 PR for issue #4056 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented Oct 18, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 974b41d
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/6352a5bf70f4fa00083e8e8a
😎 Deploy Preview https://deploy-preview-4135--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wfm
Capture d’écran 2022-10-19 à 8 32 25 AM

could you check why tests are failing?

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PLease DRY

Comment on lines 47 to 58
{{ $t('Floor') }} :
<TokenMoney
v-if="tokenId"
:value="collectionFloorPrice"
:token-id="tokenId"
:prefix="urlPrefix"
inline />
<Money
v-else
:value="collectionFloorPrice"
inline
data-cy="collection-floor-price" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unify under one component (check AccountBalance.vue)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I made a CommonTokenMoney Component.

@@ -18,4 +19,8 @@ export default class PrefixMixin extends Vue {
get isMoonriver(): boolean {
return this.urlPrefix === 'movr'
}

get tokenId() {
return getKusamaAssetId(this.urlPrefix)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getKusamaAssetId should handle also other chains

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, the CommonTokenMoney could handle both rmrk\bsx\snek chains. What else chain should I add? 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the future there will be movr/glmr and astar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, but I'm not sure which token should be used in these chains.

I guess the asset list query should also add more tokens in the future. Or if I have miss something?
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return zero for now please

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or better we can define that in the chain config :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will check.

@Jarsen136 Jarsen136 marked this pull request as draft October 19, 2022 16:13
@Jarsen136 Jarsen136 marked this pull request as ready for review October 20, 2022 01:12
@@ -0,0 +1,26 @@
<template>
<TokenMoney
v-if="tokenId"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokenId isnt prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokenId is from PrefixMixin rather than the props

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokenId is props now

@yangwao yangwao removed the request for review from petersopko October 21, 2022 09:21
@yangwao
Copy link
Member

yangwao commented Oct 21, 2022

hmm, I guess it's not possible to have bsx on rmrk :)

image

@codeclimate
Copy link

codeclimate bot commented Oct 21, 2022

Code Climate has analyzed commit 974b41d and detected 0 issues on this pull request.

View more on Code Climate.

@Jarsen136
Copy link
Contributor Author

hmm, I guess it's not possible to have bsx on rmrk :)

Good catch! Gallery items from different chains may display on the Newest list at the same time.

✅ FIXED
image

@yangwao
Copy link
Member

yangwao commented Oct 21, 2022

1.0000 KSM ? Like should be 1 KSM I guess?

image

@Jarsen136
Copy link
Contributor Author

1.0000 KSM ? Like should be 1 KSM I guess?

on bsx:

image

on rmrk:

image

It seem related to #3868

@yangwao
Copy link
Member

yangwao commented Oct 24, 2022

hmm so till then I guess we can merge it?

@yangwao
Copy link
Member

yangwao commented Oct 24, 2022

let's see

pay 50

@yangwao
Copy link
Member

yangwao commented Oct 24, 2022

😍 Perfect, I’ve sent the payout
💵 $50 @ 34.44 USD/KSM ~ 1.452 $KSM
🧗 Caiv9TbPz68q5dC8EcHu5xKYPRnremimGzqmEejDFNpWWLG
🔗 0x0cddd54ad5108d54817ded1dd3fa96ce06ed92713e5b3b45f65b223aac85221e

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Oct 24, 2022
@yangwao yangwao merged commit 9ada62b into kodadot:main Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correct currency displayed in profiledropdown
5 participants