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

feat: featured collections carousel #494

Merged

Conversation

patricio0312rev
Copy link
Contributor

@patricio0312rev patricio0312rev commented Nov 23, 2023

[featured collections] add featured collections slider

Summary

  • Featured collections are now being displayed in a hero carousel on /collections overview page.
  • The NFTs for our Featured collections are being cached and we will update them every 12 hours.
  • NftSeeder has been updated to add support for description and is_featured fields.
  • New CollectionFeaturedData data type that will be used to get the new data structure for featured collections.
  • New entries in our language files to add support for new texts in use.
  • The featured-collections-overlay class name has been added, to add support for the gradient for our featured collections card's background.
  • New prop for GridHeader. It now supports custom wrapperClassNames.
  • CollectionNft card now supports custom class names.
  • New FeaturedCollectionsCarousel and FeaturedCollectionsItem components.
  • FormatCrypto component now supports maximumFractionDigits.
  • Unit tests have been added for component features.
  • The autoplay effect has been added to our carousel. It will change the slide every 5 secs.

NOTE: Changes related to arrows and pagination for the sliders will come in other ticket.

Steps to reproduce

  • Run the app in local or testing_e2e mode.
  • Make sure your testing wallet has some NFTs. Or you can just run composer db:dev.
  • If you're not running migrations, or if you want to toggle featured collections, go to /admin portal.
  • Click on Collections and mark as featured 3 or 4 collections.
    • I recommend to test for: collection with +3 nfts + description, collection with 2 nfts , collection with no description, collection with just 1 nft.
  • Save the changes.
  • Go to /collections page and see the magic ✨
Screen.Recording.2023-11-23.at.10.07.30.mov

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I opened a corresponding card on Clickup for any remaining TODOs in my code
  • I added a short description on how to test this PR (if necessary)
  • I added a storybook entry for the component that was added (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ItsANameToo ItsANameToo added this to the TBD milestone Nov 24, 2023
@patricio0312rev patricio0312rev marked this pull request as ready for review November 24, 2023 12:04
Copy link

@samharperpittam samharperpittam left a comment

Choose a reason for hiding this comment

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

Th component is missing a border around it.

Design
image

Imp
image

@samharperpittam samharperpittam marked this pull request as draft November 24, 2023 12:19
Copy link

@samharperpittam samharperpittam left a comment

Choose a reason for hiding this comment

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

Very unlikely scenario in the real world, but if a featured Collection just contains 1 NFT, the page isn't formatted correctly at 768 down

image

Copy link

@samharperpittam samharperpittam left a comment

Choose a reason for hiding this comment

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

On mobile, the carousel can be different heights depending if a Collection description is displayed or not. Might be worth using a fixed height at all time. See video

Screen.Recording.2023-11-24.at.12.30.42.mov

@patricio0312rev
Copy link
Contributor Author

Fixes have been implemented 💜

Screen.Recording.2023-11-24.at.09.57.09.mov

@patricio0312rev patricio0312rev marked this pull request as ready for review November 24, 2023 15:07
Copy link
Contributor

@goga-m goga-m left a comment

Choose a reason for hiding this comment

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

@patricio0312rev overall looks good 💯

Similar to Sam's finding, also noticed is that in smaller screens when you have 1 nft it occupies all the available space which results in having different heights when you have slides that have > 1 nft along with those with only 1 nft

E.g with 1 nft:
2023-11-24-165614_1073x1218_scrot

And with more (also notice that gap that this one has, it's because the slider takes the height of the highest slide):

2023-11-24-165313_1070x1201_scrot

Perhaps it makes sense to limit the size of the nft card to be as when there are multiple, so that we have the same heights when changing slides 🤔

cc @ItsANameToo @samharperpittam

@patricio0312rev
Copy link
Contributor Author

@goga-m pretty sure we fixed that this morning. Did you pull latest changes? 👀 😰

@goga-m
Copy link
Contributor

goga-m commented Nov 27, 2023

@patricio0312rev indeed didn't have the latest changes when tested. Looking good 💯

slider-2023-11-27_10.38.27.mp4

@ItsANameToo ItsANameToo merged commit ae32435 into feat/collections-overview Nov 27, 2023
16 checks passed
@ItsANameToo ItsANameToo deleted the feat/featured-collections-slider branch November 27, 2023 09:49
@ItsANameToo ItsANameToo modified the milestones: TBD, 0.13.0 Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants