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

Add check for blogpostcard if in carousel or grid #1149

Merged
merged 4 commits into from
Jan 12, 2024

Conversation

daksas303
Copy link
Contributor

  • Cards are not full height in "Suvisiaci obsah" #1143
  • The issue seemed to be that Safari is unable to figure out the real height when a card is used in a grid. Added check and if BlogPostCard is used inside a carousel, then use full height and if in a grid then grid handles itself the height of the cards.

Copy link
Contributor

@radoslavzeman radoslavzeman left a comment

Choose a reason for hiding this comment

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

Thank you for finding the issue.

Regarding to the solution, a component should not be dependent on the parent component and behave differently in a specific component (as BlogCard would behave differently in Carousel)

Better approach would be if Carousel gives some classes to its children. Specifically, <li> element should give its child "h-full" class.
In tailwind, you can address children like this [&>*]:h-full.

Please try to solve the issue as suggested and test properly.

@github-actions github-actions bot added pr: needs work 🛠️ Changes requested before another review and removed pr: needs review 🙏 labels Dec 20, 2023
@daksas303 daksas303 added pr: needs review 🙏 and removed pr: needs work 🛠️ Changes requested before another review labels Dec 20, 2023
Copy link
Contributor

@Ty-ci Ty-ci left a comment

Choose a reason for hiding this comment

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

All clear, well done 👍
One curious question added in comment

@@ -16,7 +17,7 @@ const EventCardShowcase = () => {
dateTo="2023-05-12T15:00:00Z"
imageSrc="https://cdn-api.bratislava.sk/strapi-homepage/upload/silvester_odpocitavanie_f45d8e6629.jpg"
imageSizes={generateImageSizes({ default: '272px', lg: '384px' })}
className="w-[272px] lg:w-96"
className="h-[14.5rem] w-[272px] lg:h-[18.75rem] lg:w-96"
Copy link
Contributor

@Ty-ci Ty-ci Jan 9, 2024

Choose a reason for hiding this comment

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

Why combine rem and px?
(also on line 30)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just didn't care, because it's only in styleguide.

@github-actions github-actions bot added pr: fix & ship 🚢 No additional review needed before merge - some work may be required, if specified in last review and removed pr: needs review 🙏 labels Jan 9, 2024
@radoslavzeman radoslavzeman enabled auto-merge (squash) January 9, 2024 16:00
Copy link

github-actions bot commented Jan 9, 2024

Test build pipeline info 🚀

Changes in the code and tag info:

➡️ Changes in strapi: false

➡️ Changes in next: true

We are going to build 🚢

🔜 next

🥳 Bratiska-cli successfully created an kustomize file.
🥳 Bratiska-cli successfully built an image from path: next/

@radoslavzeman radoslavzeman merged commit acbfed6 into master Jan 12, 2024
14 checks passed
@radoslavzeman radoslavzeman deleted the FIX/cards-not-full-height branch January 12, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix & ship 🚢 No additional review needed before merge - some work may be required, if specified in last review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cards are not full height in "Suvisiaci obsah" Article cards overflow/shrink on Safari
3 participants