-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
#4496 gallery card changes #4511
Conversation
✅ Deploy Preview for koda-nuxt ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Oh no.. CarouselCard, I find it misleading.
Can't we have a fresh GalleryCard
component?
Or can we rename carousel card and use it in both places? Since the ui and functionality is totally same |
if the design is exactly the same I'm not against, as long as we don't end up with special cases, I'm fine with it. |
|
@roiLeo so I shifted the gallery card to histoire but I haven't used that yet for carousel, I feel like shifting carousel should be done in a new issue as there is some logical part for that, that might need some moving too |
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.
Hey! Why did you change the carousel code if the outcome is not affected?
what needs to change in histoire component:
- use placeholder/fixed data
- don't use nuxt
- don't use buefy helpers
- rename
NftCard
toNeoNftCard
?
I don't see the point of components/rmrk/Gallery/NftCard.vue
file you can directly import the component
@prachi00 Can we please move it outside of |
I just did the UI changes for the carousel card that should be according to the design |
@roiLeo by buefy helpers, do you mean buefy css classes? is-flex and all? Also, using histoire newNftCard in a component that is not in vue compostion (gallery.vue) doesnt work, that is why I created a new composition component and used it there |
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.
@roiLeo by buefy helpers, do you mean buefy css classes? is-flex and all?
Nvm since we use Bulma theme in histoire it's working.
Also, using histoire newNftCard in a component that is not in vue compostion (gallery.vue) doesnt work, that is why I created a new composition component and used it there
lol, Nice move! I didn't knew that Gallery wasn't rewritten in Vue3 way.
- What's the difference between
NeoNftCard
&NftCard
? if one of them is not used please delete it - Could you please create story for each component you create?
components/rmrk/Gallery/NftCard.vue
stil insidermrk
folder
so NftCard is in vue composition, so it is using NeoNftCard which gallery is not able to yet as it isnt in vue3, I guess we can remove NftCard once gallery is in vue3 and then directly use NeoNftCard in gallery |
I'm referring to |
@roiLeo I added the story file, but I dont know how to see it, I ran story:dev |
Code Climate has analyzed commit 546c0fa and detected 0 issues on this pull request. View more on Code Climate. |
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.
✅ let's move on
pay 80 usd |
😍 Perfect, I’ve sent the payout 🪅 Let’s grab another issue and get rewarded! |
Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.
PR Type
Context
Before submitting pull request, please make sure:
Optional
Had issue bounty label?
Payout
Community participation
Screenshot 📸