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: More cards on mobile carousels #9135

Merged
merged 3 commits into from
Jan 27, 2024
Merged

Conversation

hassnian
Copy link
Contributor

@hassnian hassnian commented Jan 25, 2024

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Needs Design check

Did your issue had any of the "$" label on it?

  • Fill up your DOT address: Payout

Screenshot 📸

  • My fix has changed something on UI;

CleanShot 2024-01-25 at 12 30 23

@hassnian hassnian requested a review from a team as a code owner January 25, 2024 07:22
@hassnian hassnian requested review from preschian and Jarsen136 and removed request for a team January 25, 2024 07:22
Copy link

netlify bot commented Jan 25, 2024

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit cc6e42f
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/65b3309d66d9e6000855c021
😎 Deploy Preview https://deploy-preview-9135--koda-canary.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 configuration.

@kodabot
Copy link
Collaborator

kodabot commented Jan 25, 2024

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

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Jan 25, 2024
Copy link
Contributor

reviewpad bot commented Jan 25, 2024

AI-Generated Summary: This pull request contains changes across three files that primarily focus on the design of a Carousel component within a NFT application. The changes include additional minimal styling for the carousel and modifications in CarouselInfo. Also, the carousel's appearance on mobile is accounted for by adjusting the number of slides and spacing shown. Added control over the display of NFT card information within the carousel is provided, enabling a minimal display mode that reduces the information shown on smaller screens. Notably, the code for the CarouselAgnostic.vue and CarouselInfo.vue components has been modified to introduce a minimal carousel display option. This mode adjusts the provided information, padding, and other display attributes to appear optimally on smaller screens. In the _carousel.scss stylesheet, a new minimal class is added for controlling the padding. Further, breakpoints have been adjusted to optimize the carousel's display across various screen sizes.

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.

any way to do it without passing minimal prop?

@hassnian
Copy link
Contributor Author

any way to do it without passing minimal prop?

without a prop called minimal ?
maybe what nftcarddoes with a prop variant 'primary'| 'minimal'

or using a slot?

@exezbcz
Copy link
Member

exezbcz commented Jan 25, 2024

there should not even be the network IMO

image - this is the card from explore

in old explorer v0.9 figma file its without

image

  • its not that important on mobile to see what network is it

@preschian
Copy link
Member

any way to do it without passing minimal prop?

without a prop called minimal ? maybe what nftcarddoes with a prop variant 'primary'| 'minimal'

or using a slot?

Use $device.isMobile? so we can drop the prop

Copy link

sonarcloud bot commented Jan 26, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

codeclimate bot commented Jan 26, 2024

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

View more on Code Climate.

@exezbcz
Copy link
Member

exezbcz commented Jan 26, 2024

loooks okay!
image

Copy link
Member

@preschian preschian left a comment

Choose a reason for hiding this comment

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

lgtm. It can be a follow-up issue for the CSS helper (isMobile). Related:

@yangwao
Copy link
Member

yangwao commented Jan 27, 2024

Thanks!
pay 30 usd

@yangwao yangwao merged commit 90e7f57 into kodadot:main Jan 27, 2024
15 checks passed
@yangwao
Copy link
Member

yangwao commented Jan 27, 2024

😍 Perfect, I’ve sent the payout
💵 $30 @ 6.7 USD/DOT ~ 4.478 $DOT
🧗 16faLfsywwNATaEfbH2ah75dn6ZmctQWpMS5G4KFhbmj5hnD
🔗 0xadfa9894c3cea2836c87bdc902457bea0dac6630970396083d9657551af21880

🪅 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 Jan 27, 2024
This was referenced Feb 1, 2024
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-✅ small Pull request is small waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More cards on mobile carousels
6 participants