fix: #4302 add wallet name #4476

petersopko · 2022-12-11T12:06:37Z

Thank you for your contribution to the KodaDot NFT gallery.

👇 _ Let's make a quick check before the contribution.

PR Type

Bugfix
Feature
Refactoring

Context

Closes Add name of wallet you are logged in and show assets of address #4302, Closes Total usd value of account should be calculated based on the real time price of KSM/BSX/xxx #4126

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 </bsx/collection>
I've tested PR on mobile
I've written unit tests 🧪
I've found edge cases

Had issue bounty label?

Fill up your KSM address: Payout

Community participation

Are you at KodaDot Discord?

Screenshot 📸

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

KodaDot.-.Kusama.NFT.Market.Explorer._.Low.Carbon.NFTs.-.Google.Chrome.2022-12-11.12-57-54.mp4

kodabot · 2022-12-11T12:06:40Z

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

netlify · 2022-12-11T12:07:08Z

✅ Deploy Preview for koda-nuxt ready!

Name	Link
🔨 Latest commit	`dcd3d07`
🔍 Latest deploy log	https://app.netlify.com/sites/koda-nuxt/deploys/639893210e75690009312f69
😎 Deploy Preview	https://deploy-preview-4476--koda-nuxt.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

petersopko · 2022-12-11T12:20:25Z

one thing left when it comes to functionality: changing wallet doesn't update the name in the dropdown, it updates only after refreshing the page. I've spent some time on how to do this and I have only stupid ideas, I'd appreciate some hints here 🥺

petersopko · 2022-12-11T12:21:20Z

@exezbcz btw, I have included your comments regarding the dividers etc., one question tho, shouldn't the blue color change on hover? I couldn't figure it out from the design.

exezbcz · 2022-12-11T12:30:14Z

@exezbcz btw, I have included your comments regarding the dividers etc., one question tho, shouldn't the blue color change on hover? I couldn't figure it out from the design.

yup, we can add this color on hover, it isnt in the handoff files yet.
#B6CBFF

Working file is a mess :D Visit only the handoffs

yangwao · 2022-12-11T15:58:05Z

What about balance part?

petersopko · 2022-12-11T17:25:58Z

What about balance part?

Must've missed that, I somehow didn't get this requirement from issue name / desc 😆 working on it.

yangwao · 2022-12-11T19:29:13Z

Anyway I've logged in, and this is not the name of my account :D

# Conflicts: # components/navbar/ProfileDropdown.vue

This reverts commit 408bd5f.

petersopko · 2022-12-11T21:36:23Z

Anyway I've logged in, and this is not the name of my account :D

oh okay, that's actually unrelated to this PR, also on beta, and caused by recent one here:

🐛 🔧 make identityIndex respond to address change #4467

@daiagi would you please create supplemental PR to your fix, so it doesn't break identity?

I've reverted it and it works, it seems that identity is getting passed as empty object from IdentityIndex.vue

codeclimate · 2022-12-11T23:32:10Z

utils/coingecko.ts

@@ -26,6 +26,21 @@ export const getKsmPrice = async (): Promise<void> => {
  }
 }

+export const getBsxPrice = async (): Promise<void> => {


Similar blocks of code found in 2 locations. Consider refactoring.

petersopko · 2022-12-11T23:40:01Z

needs some more styling + storing basilisk/usd price (hopefully tomorrow evening).

@yangwao when it comes to USD price previews, basilisk is listed on coingecko, but not sure about tnkr, do we want to show that here? maybe hide tnkr for now ?

yangwao · 2022-12-12T08:41:27Z

oh okay, that's actually unrelated to this PR, also on beta, and caused by recent one here:

oh good catch, my bad 🧐 haven't tested it properly everywhere

do we want to show that here? maybe hide tnkr for now ?

ah yes if price is not available we will be showing iirc "--" in current design.

yes lot of tokens on lbp at some extent won't have USD price

yangwao · 2022-12-12T23:17:05Z

let's refactor utils and let's go?

petersopko · 2022-12-13T00:00:14Z

let's refactor utils and let's go?

working on it, for some reason I can't fetch my KSM balance on RMRK. Doesn't work on kodadot.xyz, beta or local, idk.

codeclimate · 2022-12-13T00:02:04Z

store/wallet.ts

+  },
+}
+
+export const actions: ActionTree<WalletState, WalletState> = {


Similar blocks of code found in 3 locations. Consider refactoring.

codeclimate · 2022-12-13T00:02:04Z

store/wallet.ts

+  getWalletName: ({ wallet }: any) => wallet.name,
+}
+
+export const mutations: MutationTree<WalletState> = {


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2022-12-13T00:02:04Z

store/wallet.ts

+
+export type WalletState = ReturnType<typeof state>
+
+export const getters: GetterTree<WalletState, WalletState> = {


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2022-12-13T00:39:08Z

Code Climate has analyzed commit 358fba5 and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	5

View more on Code Climate.

petersopko · 2022-12-13T01:06:19Z

currently testable at snek/bsx with various wallets.

todo:

get ksm balance and show it in similar fashion. as I described, I'm currently not able to fetch KSM balance, it seems to be broken at prod and beta as well
hide the whole asset section in case there are only 0 balance assets
cleanup + style touchup

daiagi · 2022-12-13T06:30:52Z

@petersopko
I've been exploring the issue you mentioned where my PR broke onchain identity and:

you were right, it does.
I have yet to understand why it breaks it or how to fix it

one thing left when it comes to functionality: changing wallet doesn't update the name in the dropdown, it updates only after refreshing the page. I've spent some time on how to do this and I have only stupid ideas, I'd appreciate some hints here pleading_face

same here

petersopko · 2022-12-13T06:39:58Z

same here

I have solved issue with reactive wallet name by storing the wallet name in store, which is reactive when accessed through getters in components, i.e.

  get userWalletName(): string {
    return this.$store.getters['wallet/getWalletName']
  }

yangwao · 2022-12-13T12:20:25Z

why is there prefix My ?

todo:

yes, we can do follow-up, 14 changed files are bit frightening to me 😅 this never went well with complex changes

Otherwise is mergable I guess?

components/identity/module/IdentityLink.vue

components/navbar/ProfileDropdown.vue

roiLeo · 2022-12-13T12:30:16Z

components/navbar/ProfileDropdown.vue

        <Identity
          :address="account"
          class="navbar__address is-size-6"
          hide-identity-popover />
      </b-dropdown-item>

-      <hr class="dropdown-divider mx-4" aria-role="menuitem" />


I personally would not have moved <hr /> tags to component

roiLeo · 2022-12-13T12:34:05Z

components/rmrk/Profile/ProfileAssetsList.vue

+          <th class="has-text-grey">Asset</th>
+          <th class="has-text-grey">Balance</th>


Suggested change

<th class="has-text-grey">Asset</th>

<th class="has-text-grey">Balance</th>

<th class="has-text-grey">{{ $t('assets') }}</th>

<th class="has-text-grey">{{ $t('general.balance') }}</th>

would it be possible to move this file outside `/rmrk' folder?

styles/themes/_dark.scss

petersopko · 2022-12-13T13:27:54Z

why is there prefix My ?

I thought that's what was in design, so it should be just ${walletName} wallet? Or just ${walletName}?

todo:

yes, we can do follow-up, 14 changed files are bit frightening to me 😅 this never went well with complex changes

Otherwise is mergable I guess?

Not mergeable yet, please let me implement feedback by @roiLeo 🙏🏻 and clean up a little.

yangwao · 2022-12-13T14:24:26Z

just ${walletName}?

yes

roiLeo · 2022-12-13T15:03:20Z

locales/en.json

+    "asset": "Asset",
+    "balance": "Balance",
+    "noAssets": "There are currently no assets in this wallet."


I would move asset & noAssets in

"general": { ... }

yangwao · 2022-12-13T15:13:29Z

Seems https://github.com/kodadot/nft-gallery/pull/4476/files#diff-a4a33e7ecfb7969dbf41700112aa3cf6f2880fe4a397f3faececd4a001e67384 is copy of https://github.com/kodadot/nft-gallery/blob/main/components/bsx/Asset/AssetList.vue ?

I mean, I'm a bit against copying functions over file and duplicating code.

I'm merging it because this was the very last issue to have it released; otherwise, I dislike the code approach ignoring DRY approach.

pay 60 USD

yangwao · 2022-12-13T15:13:42Z

😍 Perfect, I’ve sent the payout
💵 $60 @ 29.12 USD/KSM ~ 2.06 $KSM
🧗 FNyt7T1xdbhN7dagf7yGYCRJuE3R45VmTCE3tjzy1rKxa7y
🔗 0x190ecd6efea6a858e7132ad5d3c3418a91daa43f1824e71ca3ef9d1d61d66f9b

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

petersopko · 2022-12-13T16:01:03Z

dislike the code approach ignoring DRY approach

pay 60 USD

I'm not too fond of the state in which it got merged in either, and I wish I had time to clean up/finish. Maybe next time :)

petersopko added 7 commits December 11, 2022 12:48

✨ wallet name

c1df0cc

✨ identity link

c3c824c

✨ copy to clipboard

b01f80e

⚡ dynamic link

8c7581f

💄 style touchup + small divider fix

8e4c6db

💄 small change

d06fce2

⚡ computed explorer link

74c5be9

petersopko requested a review from a team as a code owner December 11, 2022 12:06

petersopko requested review from preschian and removed request for a team December 11, 2022 12:06

💄 add on hover style

d6fe9be

✨ assets here

1aa2c4e

petersopko added 2 commits December 11, 2022 18:27

🚧 assets here 2

4683036

⚡ poc assets in dropdown

c869be9

yangwao added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Dec 11, 2022

petersopko added 2 commits December 11, 2022 21:54

Merge branch 'main' into 4302-add-wallet-name-clipboard

f873bb9

# Conflicts: # components/navbar/ProfileDropdown.vue

Revert ":bug: :wrench: make identityIndex respond to address change"

7bae321

This reverts commit 408bd5f.

✨ added usd price for ksm, started bsx

a6aad8d

codeclimate bot reviewed Dec 11, 2022

View reviewed changes

petersopko added 6 commits December 12, 2022 20:30

Merge branch 'main' into 4302-add-wallet-name-clipboard

8225ac5

⚡ hide zero-balance assets

3e1eb78

⚡ reactive wallet name in store

57579ff

✨ basilisk usd value

92f7564

🐛 reliable asset filtering

97d7d71

⚡ cleanup + snek

9d2e215

codeclimate bot reviewed Dec 13, 2022

View reviewed changes

petersopko added 3 commits December 13, 2022 01:24

🔥 simpler gecko

044de9b

🚧 rmrk placeholder

2582fcb

🚧 nonsnek placeholder

358fba5

roiLeo reviewed Dec 13, 2022

View reviewed changes

petersopko added 2 commits December 13, 2022 15:56

⚡ dividers, styling, rmrk simple, translation

5e7ef80

Merge branch 'main' into 4302-add-wallet-name-clipboard

dcd3d07

roiLeo reviewed Dec 13, 2022

View reviewed changes

yangwao added the paid pull-request has been paid label Dec 13, 2022

yangwao merged commit e22bdde into kodadot:main Dec 13, 2022

roiLeo mentioned this pull request Dec 13, 2022

🔧 en translation keyword #4486

Merged

roiLeo mentioned this pull request Dec 14, 2022

⬆️ upgrade sub-api dependencies #4501

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #4302 add wallet name #4476

fix: #4302 add wallet name #4476

petersopko commented Dec 11, 2022 •

edited

kodabot commented Dec 11, 2022

netlify bot commented Dec 11, 2022 •

edited

petersopko commented Dec 11, 2022

petersopko commented Dec 11, 2022

exezbcz commented Dec 11, 2022

yangwao commented Dec 11, 2022

petersopko commented Dec 11, 2022

yangwao commented Dec 11, 2022

petersopko commented Dec 11, 2022

codeclimate bot Dec 11, 2022

petersopko commented Dec 11, 2022

yangwao commented Dec 12, 2022 •

edited

yangwao commented Dec 12, 2022

petersopko commented Dec 13, 2022

codeclimate bot Dec 13, 2022

codeclimate bot Dec 13, 2022

codeclimate bot Dec 13, 2022

codeclimate bot commented Dec 13, 2022

petersopko commented Dec 13, 2022

daiagi commented Dec 13, 2022 •

edited

petersopko commented Dec 13, 2022

yangwao commented Dec 13, 2022

roiLeo Dec 13, 2022

roiLeo Dec 13, 2022

roiLeo Dec 13, 2022

petersopko commented Dec 13, 2022

yangwao commented Dec 13, 2022

roiLeo Dec 13, 2022

yangwao commented Dec 13, 2022

yangwao commented Dec 13, 2022

petersopko commented Dec 13, 2022


		export type WalletState = ReturnType<typeof state>

		export const getters: GetterTree<WalletState, WalletState> = {

		<th class="has-text-grey">Asset</th>
		<th class="has-text-grey">Balance</th>

fix: #4302 add wallet name #4476

fix: #4302 add wallet name #4476

Conversation

petersopko commented Dec 11, 2022 • edited

PR Type

Context

Before submitting pull request, please make sure:

Optional

Had issue bounty label?

Community participation

Screenshot 📸

kodabot commented Dec 11, 2022

netlify bot commented Dec 11, 2022 • edited

✅ Deploy Preview for koda-nuxt ready!

petersopko commented Dec 11, 2022

petersopko commented Dec 11, 2022

exezbcz commented Dec 11, 2022

yangwao commented Dec 11, 2022

petersopko commented Dec 11, 2022

yangwao commented Dec 11, 2022

petersopko commented Dec 11, 2022

codeclimate bot Dec 11, 2022

Choose a reason for hiding this comment

petersopko commented Dec 11, 2022

yangwao commented Dec 12, 2022 • edited

yangwao commented Dec 12, 2022

petersopko commented Dec 13, 2022

codeclimate bot Dec 13, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 13, 2022

Choose a reason for hiding this comment

codeclimate bot Dec 13, 2022

Choose a reason for hiding this comment

codeclimate bot commented Dec 13, 2022

petersopko commented Dec 13, 2022

daiagi commented Dec 13, 2022 • edited

petersopko commented Dec 13, 2022

yangwao commented Dec 13, 2022

roiLeo Dec 13, 2022

Choose a reason for hiding this comment

roiLeo Dec 13, 2022

Choose a reason for hiding this comment

roiLeo Dec 13, 2022

Choose a reason for hiding this comment

petersopko commented Dec 13, 2022

yangwao commented Dec 13, 2022

roiLeo Dec 13, 2022

Choose a reason for hiding this comment

yangwao commented Dec 13, 2022

yangwao commented Dec 13, 2022

petersopko commented Dec 13, 2022

petersopko commented Dec 11, 2022 •

edited

netlify bot commented Dec 11, 2022 •

edited

yangwao commented Dec 12, 2022 •

edited

daiagi commented Dec 13, 2022 •

edited