-
-
Notifications
You must be signed in to change notification settings - Fork 360
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
Extend Money Component to show fiat value #175
Conversation
Hey @JKrupinski, I would extend the |
@vikiival Do you mean extend '@/components/shared/format/Money.vue'; component, add showFiatValue as a prop and import in my KusamaPrice component? or am I wrong |
@JKrupinski yup, I guess so. I mean showing price without context doesn't make any sense, so having it next to the value of NFT would give most of the contextual sense to me :) Don't forget to post your KSM address! :) |
@vikiival please review my code |
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.
Please fix those minor things.
On the other hand you made awesome job!
@@ -25,6 +35,33 @@ export default class Money extends Vue { | |||
return this.chainProperties.tokenSymbol | |||
} | |||
|
|||
} | |||
public created() { |
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.
use mounted
instead of created
.
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 but I don't know why do you want to use mounted
here
with created
hook we can get data faster
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 always used mounted for fetching. However, it looks like it doesn't matter.
https://stackoverflow.com/a/57089636
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 always use created
for fetching because it's executed before mounted and I don't need DOM elements here
|
||
private async getFiatValue() { | ||
try { | ||
const { data } = await axios.get(`${this.apiUrl}/simple/price`, { |
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 would move this to separate file src/utils/coingecko.ts
or src/coingecko.ts
or under src/fetch.ts
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.
moved to src/coingecko.ts
please check if it is that you want, I've added just new axios instance with default url to coingecko
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.
If I misunderstood it and you wanted to move whole request to separate file just let me know
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.
Yeah I just wanted to move it so separate file
</script> | ||
|
||
<style lang="scss"> |
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.
Wow nice !
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.
simple solution 😄
This is related to #83
Where do you want to show kusama price?
Can I use coingecko api to get it? (Rate Limit: 100 requests/minute)
For now it looks like this (bottom right corner)