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

Collection redesign under banner section #5120

Merged

Conversation

daiagi
Copy link
Contributor

@daiagi daiagi commented Feb 26, 2023

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

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

Screenshot 📸

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

image
image
image

@daiagi daiagi requested a review from a team as a code owner February 26, 2023 10:22
@daiagi daiagi requested review from roiLeo and Jarsen136 and removed request for a team February 26, 2023 10:22
@kodabot
Copy link
Collaborator

kodabot commented Feb 26, 2023

WARNING @daiagi PR for issue #5073 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #5073

@netlify
Copy link

netlify bot commented Feb 26, 2023

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit c57124c
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/64055d2fd0c8270008e7f4f1
😎 Deploy Preview https://deploy-preview-5120--koda-nuxt.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@exezbcz
Copy link
Member

exezbcz commented Feb 26, 2023

hey hey, nice job

  • I think we can stretch the description even more, so it won´t take too much space vertically
    image
  • Afaik there is also a limit so it wont get any longer - solution would also be too put "see more" collapsible option so the screen is not only about the description - don´t implement this, I am just thinking out loud :D

just to be sure, the social icons are going to be over the banner

@daiagi
Copy link
Contributor Author

daiagi commented Feb 26, 2023

I think we can stretch the description even more, so it won´t take too much space vertically

agree, will do

just to be sure, the social icons are going to be over the banner

yep, i just waited for the banner to be merged

a couple of follow up questions:

  1. RMRK doesn't have offers (does it?) what should we place there instead? resolved by removing best offers line where offers don't exist
  2. @kodadot/internal-dev where can i get the info of which network the collection belongs to?
  3. is "best offer" should be the best offer for the entire collection or for an individual NFT in the collection? for an individual NFT

thanks

components/collection/CollectionInfo.vue Outdated Show resolved Hide resolved
components/collection/CollectionInfo.vue Outdated Show resolved Hide resolved
components/collection/CollectionInfo.vue Outdated Show resolved Hide resolved
components/collection/CollectionInfo.vue Outdated Show resolved Hide resolved
components/collection/CollectionInfo.vue Outdated Show resolved Hide resolved
@exezbcz
Copy link
Member

exezbcz commented Feb 26, 2023

@daiagi
thanks!
nope, rmrk does not have offers - we can remove the line imo

best offer in this case works like floor price - its a best offer across all nfts - so if one nft has offer and the other dont, the best offer is the one. Its more general approach because we dont have collection offers yet

@daiagi
Copy link
Contributor Author

daiagi commented Feb 27, 2023

nope, rmrk does not have offers - we can remove the line imo

Done

best offer in this case works like floor price - its a best offer across all nfts - so if one nft has offer and the other dont, the best offer is the one. Its more general approach because we dont have collection offers yet

Got it, Thank you 👍

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

from what I can see on design, first row:

  • creator name & collection desription should be in a sized 6 column
  • collection info line should be in a sized 3 column align to right (offset-3)

note: preview failed?

@daiagi
Copy link
Contributor Author

daiagi commented Feb 27, 2023

from what I can see on design, first row:

* creator name & collection desription should be in a sized 6 column

* collection info line should be in a sized 3 column align to right (offset-3)

got it, ill fix
edit: the grid system gave me troubles, I've implemented instead using max-width

note: preview failed?

yeah, I'm not sure why, I didn't make any changes that would fail a build
edit: preview passing now

also @roiLeo , do you have input for me as to how to determine which chain a collection belongs to?

@roiLeo
Copy link
Contributor

roiLeo commented Feb 27, 2023

got it, ill fix edit: the grid system gave me troubles, I've implemented instead using max-width

Oh no... what happened? have you tried to wrap in columns?

also @roiLeo , do you have input for me as to how to determine which chain a collection belongs to?

Why would you need that for? can't you check with prefix?

@daiagi
Copy link
Contributor Author

daiagi commented Feb 27, 2023

Oh no... what happened? have you tried to wrap in columns?

yes, i did, the content overflows the grid and breaking the layout
I think it's because I've set a min-padding between field name and value (such as Network: Kusama)
which is mandated by the design
It looks good to me as is right now

also @roiLeo , do you have input for me as to how to determine which chain a collection belongs to?

Why would you need that for? can't you check with prefix?

a user can change the network using the network dropdown, which will change the text in the collection description, but surely the collection doesn't belong to all networks, does it?

image
image

@roiLeo
Copy link
Contributor

roiLeo commented Feb 27, 2023

yes, i did, the content overflows the grid and breaking the layout

I can see this is doable, I'll try something or we can refactor it latter

a user can change the network using the network dropdown, which will change the text in the collection description, but surely the collection doesn't belong to all networks, does it?

We don't allow to change network on collection page, I don't know what you are talking about since I don't see Network selector on design.
Collection Id is by network.

By the way no-wrap class already exist and is available globally

@daiagi
Copy link
Contributor Author

daiagi commented Mar 1, 2023

Mergable?

@daiagi daiagi requested a review from vikiival March 3, 2023 04:14
@daiagi daiagi mentioned this pull request Mar 3, 2023
17 tasks
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

Mergable?

might be related with kodadot/snek#142 (or kodadot/snek#33)

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

👁️

@daiagi
Copy link
Contributor Author

daiagi commented Mar 5, 2023

please use config map :) *.config.ts

✔️ done @vikiival

IMO we should show full numbers

✔️ done @roiLeo @exezbcz
image

@daiagi daiagi requested a review from vikiival March 5, 2023 08:33
@codeclimate
Copy link

codeclimate bot commented Mar 6, 2023

Code Climate has analyzed commit c57124c and detected 0 issues on this pull request.

View more on Code Climate.

@daiagi daiagi requested a review from vikiival March 6, 2023 03:26
@vikiival vikiival merged commit b45c725 into kodadot:main Mar 6, 2023
@vikiival
Copy link
Member

vikiival commented Mar 6, 2023

pay 50 usd

@yangwao
Copy link
Member

yangwao commented Mar 6, 2023

😍 Perfect, I’ve sent the payout
💵 $50 @ 34.87 USD/KSM ~ 1.434 $KSM
🧗 EfmnRhHaQqfT3phm4cUCHCU3gFVDoSPR1U9WXzMRQBMqZ4L
🔗 0x69d87605cdeacb1e1eb7ee15c42e0ade03be9ed70f1bff4868e528800b880811

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

@yangwao yangwao added the paid pull-request has been paid label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid S-visual-ok-✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bellow banner section
7 participants