-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
✨ Collection Activity Tab #5345
Conversation
✅ Deploy Preview for koda-nuxt ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
…llection-activity-tab
const mintInteraction = events[0] | ||
const mintTimeStamp = new Date(mintInteraction.timestamp).getTime() | ||
|
||
owners[nft.currentOwner] = |
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.
Similar blocks of code found in 2 locations. Consider refactoring.
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.
@vikiival seriously?
.flat() | ||
} | ||
|
||
const getOwners = (nfts) => { |
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.
Function getOwners
has 38 lines of code (exceeds 25 allowed). Consider refactoring.
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.
@vikiival I don't think that there will be anything gained by trying to reduce it farther, same goes for getFlippers
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.
update: managed to reduce getFlippers to 38
should be fixed with this #5398 |
.fixed-width { | ||
width: 66px; | ||
} | ||
.fixed-height { | ||
height: 22px; | ||
} | ||
.height-50px { | ||
height: 50px; | ||
} |
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.
do we need to set fixed height & width?
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.
long discussion about this with @exezbcz in discord...
yes
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.
altough fixed height can probably be dropped
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.
they look pretty random numbers, wdyt about changing this to inner spacing
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.
is that for the tags? :D
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.
dropping fixed height causes height diff between desktop and tablet view
i guess min-height could solve it, but what would be the point of that then?
components/collection/activity/ownerInsightsTabs/NFTSDetailsDropdown/Flipper.vue
Outdated
Show resolved
Hide resolved
components/collection/activity/ownerInsightsTabs/NFTSDetailsDropdown/Flipper.vue
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,122 @@ | |||
<template> | |||
<b-collapse |
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.
NeoCollapse
?
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.
moving it to the follow up issue with your permission
} | ||
|
||
|
||
//colors |
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.
have you seen in styles/abstracts/variables.scss
it should do the work you looking for
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.
honestly i could not figure it out
…llection-activity-tab
…ft-gallery into collection-activity-tab
Code Climate has analyzed commit 9f4865d and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Continue @daiagi here |
Isn't it p4? |
@daiagi I think this one is the issue we should focus on |
Thank you for your contribution to the KodaDot NFT gallery.
@yangwao Deliverd on time?
👇 _ Let's make a quick check before the contribution.
PR Type
Context
Before submitting pull request, please make sure:
Optional
Had issue bounty label?
Community participation
Screenshot 📸
tested on these collections: