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

Feature/show dao token balance #332

Merged
merged 6 commits into from
Sep 24, 2023
Merged

Feature/show dao token balance #332

merged 6 commits into from
Sep 24, 2023

Conversation

jagracar
Copy link
Member

Hi! This PR adds the TEIA balance info in the user profile. I hope i followed the standard way to do it!

@jagracar jagracar requested review from xat and melMass September 24, 2023 13:30
@github-actions
Copy link

github-actions bot commented Sep 24, 2023

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-09-24 20:03 UTC

.catch(error => console.log('Error while querying the account token balance:', error))

return response?.data[0]? parseInt(response.data[0]) / 1e6 : 0
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably can also use teztok here? I guess it doesn't matter that much

Copy link
Member

Choose a reason for hiding this comment

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

Nope it's actually not indexed and IIRC @xat mentionned it not being very adequate to store there so this is totally fine

Copy link
Member

@melMass melMass left a comment

Choose a reason for hiding this comment

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

Looks perfectly fine to me! I would just handle error on awaiting for the Dao token so that it doesn't bork everything if tzkt is down and either display an error dialog or just default to -1, 0, X or ERROR for the count value displayed (or even hide it if missing simply)

src/data/api.ts Outdated
'account': walletAddr,
"select": 'balance'}
const response = await axios.get(`https://api.tzkt.io/v1/tokens/balances`, { params: parameters })
.catch(error => console.log('Error while querying the account token balance:', error))
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this not doing what you want @melMass ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will return 0 tokens, unless i did something wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the tzkt url be changed to VITE_TZKT_API ? If we ever need to switch tzkt instances, then we only need to change it in https://github.com/teia-community/teia-ui/blob/main/.env

Copy link
Member

Choose a reason for hiding this comment

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

Yes indeed, I did not get that the last ? here is a conditional and not a null property fallback

return response?.data[0]? parseInt(response.data[0]) / 1e6 : 0

Are you sure the hooks are running 🤣 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can the tzkt url be changed to VITE_TZKT_API ? If we ever need to switch tzkt instances, then we only need to change it in https://github.com/teia-community/teia-ui/blob/main/.env

Did it in my last commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

Copy link
Member

@melMass melMass left a comment

Choose a reason for hiding this comment

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

Mergeable for me, I would just double check hooks are ran I find it stange that it did not add a space here before the last ?:

return response?.data[0]? parseInt(response.data[0]) / 1e6 : 0

src/pages/profile/index.jsx Show resolved Hide resolved
src/data/api.ts Outdated
'account': walletAddr,
"select": 'balance'}
const response = await axios.get(`https://api.tzkt.io/v1/tokens/balances`, { params: parameters })
.catch(error => console.log('Error while querying the account token balance:', error))
Copy link
Member

Choose a reason for hiding this comment

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

Yes indeed, I did not get that the last ? here is a conditional and not a null property fallback

return response?.data[0]? parseInt(response.data[0]) / 1e6 : 0

Are you sure the hooks are running 🤣 ?

@jagracar
Copy link
Member Author

jagracar commented Sep 24, 2023

Yes. I was wondering about the hooks... I didn't have any error when committing, so i assumed they run fine...

Maybe is because i commit directly without staging when i use eclipse?

@jagracar
Copy link
Member Author

Not sure if i'm allowed to merge... I did another change and it seems that the hooks are disabled in my commits? I have no clue how to fix that...

@melMass
Copy link
Member

melMass commented Sep 24, 2023

Not sure if i'm allowed to merge... I did another change and it seems that the hooks are disabled in my commits? I have no clue how to fix that...

What is your OS? The hooks are installed on first npm install if you are using the same local folder since a long time just clone a fresh instance and it should work (I use both windows and mac)

@jagracar
Copy link
Member Author

Not sure if i'm allowed to merge... I did another change and it seems that the hooks are disabled in my commits? I have no clue how to fix that...

What is your OS? The hooks are installed on first npm install if you are using the same local folder since a long time just clone a fresh instance and it should work (I use both windows and mac)

I use linux... I installed the repo with npm install and npm start
some how my commits overwrite the hooks...

@melMass
Copy link
Member

melMass commented Sep 24, 2023

I use linux... I installed the repo with npm install and npm start some how my commits overwrite the hooks...

You can always run prettier manually but we should find why it's doing that, I personally commit from the command line, but GUI tools shouldn't bypass it unless specifically asked to, maybe you disabled it when you initially had the issue and the config was kept or something like that

@melMass melMass merged commit 363fc57 into main Sep 24, 2023
3 checks passed
@melMass melMass deleted the feature/showDaoTokenBalance branch September 24, 2023 20:01
@jagracar
Copy link
Member Author

How do you run it locally?

npm lint ?

@jagracar
Copy link
Member Author

I cloned again the repo to be sure before my last commit and it still didn't run the hooks.

@melMass
Copy link
Member

melMass commented Sep 24, 2023

How do you run it locally?

npm lint ?

npm run format

@jagracar
Copy link
Member Author

Ok. That did work. I will use it. Even from a clean install i got 2 files that needed format changes:

  • src/styles/reset.scss
  • src/components/header/sample_events.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants