international-labour-organization / designsystem

A design system for the International Labour Organization
Apache License 2.0
1 stars 3 forks source link

Styles: Cards in group have unequal image heights #949

Open justintemps opened 2 months ago

justintemps commented 2 months ago

Cards in a card group have unequal width depending on their content. Instead, the width of all cards should be the same.

I think this is happening because the Card Group css uses 1fr to allot space whereas it should probably use percentages instead. Example: repeat(4, 25%) instead of repeat(4, 1fr)

See the cards in this example: https://www-preview.ilo.org/es/acerca-de-la-oit

Screenshot 2024-04-17 at 10 04 18 Screenshot 2024-04-17 at 10 04 02 Screenshot 2024-04-17 at 10 03 44
justintemps commented 2 months ago

Hey @Shashika6 thanks for working on this, I think I've phrased the problem incorrectly. The issue that @inesdgomes originally raised and that I misdiagnosed is that the images in the cards are different heights.

In this situation, it's clear that longer words will force cards to be unequal widths. We can't control that, but we should be able to make sure that the heights of the images inside the cards remain the same.

I'm going to close your first PR, because I don't think we should break words just to keep things the same width, that's a very blunt solution.

Instead, what I'd recommend doing is, in the feature cards, add specific flex rules on the ilo--card--image--wrapper and ilo--card--content classes. As you'll see in the below example, I did this and got ok results.

Screenshot 2024-04-17 at 14 29 58

You'll want to carefully consult the Figma designs for this component to get as close as possible to the intended ratio between the images and the content area and then do lots of testing in Storybook to make sure that there aren't any edge cases that this breaks.

Examples:

  1. What happens when there's lots of text?
  2. What happens where the image is portrait instead of landscape?

Let me know what you think.

Shashika6 commented 2 months ago

hey @justintemps, Tried implementing a fix for this with the flex rules that you provided and also tried a few other things but there is at least one case that breaks the card.

Heres how it looks like for flex 0 0 45%

  1. Large screen

Image

  1. Testing responsiveness on large screen with 4 cards.

Image

  1. One card in a large screen.

Image

justintemps commented 2 months ago

@inesdgomes see @Shashika6's comment above. This seems like a complicated problem to fix, for a minor problem that only occurs when you're trying to squeeze four cards into a space where you should really only have three. I think we should close this issue, what do you think?

GGKapanadze commented 2 months ago

Do we want to achieve something like this?

https://github.com/international-labour-organization/designsystem/assets/36326203/68c7c8ff-c054-4e12-9e53-621511750994

  1. right now, we don't have a min-width for those cards, and that's the main issue: the screen implementation you are looking at utilizes minmax
grid-template-columns: repeat(auto-fit, minmax(150px, 1fr));
  1. we are trying to pass amount of card as a class to style them correctly image image But if we want to use a grid we can do fit-content and utilize as much space as the parent gives us, or we can use flexbox with min-width too
justintemps commented 2 months ago

Thanks @GGKapanadze, the count classname determines how many cards should fit into each row, assuming that there might be more than one. So may be you have lots of space, but you still only want two cards per row for some reason.

The problem here is just that the image in the card changes height if one of the cards has to be wider than the others, because of some long word in the title.

beatrizmartinmartins commented 2 months ago

Hey team, I don't know if this could help or not but have we tried working with these breakpoints we created a while ago? This would solve the problem when we resize our screen because it should automatically move to a more "tablet" look https://www.figma.com/file/TnVsWy6VobbHzZdbHdx3Rb/ILO-Page-Templates?type=design&node-id=3980%3A44686&mode=design&t=cjmAwAxdGif8pRGI-1 @justintemps @GGKapanadze