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

[ImageItemList] Fix incorrect (below) rendering #38452

Conversation

omriklein
Copy link
Contributor

@omriklein omriklein commented Aug 13, 2023

@mui-bot
Copy link

mui-bot commented Aug 13, 2023

Netlify deploy preview

https://deploy-preview-38452--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 1a831af

RohitPaul0007

This comment was marked as off-topic.

Copy link

@RohitPaul0007 RohitPaul0007 left a comment

Choose a reason for hiding this comment

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

It will be great if you do not add this thing into this arrow function.

@omriklein
Copy link
Contributor Author

It will be great if you do not add this thing into this arrow function.

Where do you think that is should be?

@RohitPaul0007
Copy link

It will be great if you do not add this thing into this arrow function.

Where do you think that is should be?

I think you should apply the spread operator outside the function , and then you should use the "overflow" property inside the arrow function.

RohitPaul0007

This comment was marked as off-topic.

@zannager zannager added the component: image list This is the name of the generic UI component, not the React module! label Aug 14, 2023
@DiegoAndai
Copy link
Member

Hey @omriklein, thanks for working on this 🎉

I will add the link to the issue in the PR description.

May I ask you to explain why this fixes the issue? I see it fixes it, but I don't understand why. We can't merge this PR as is because the Woven example breaks (docs vs. this PR). That's why I want to try and understand the underlying issue so we can develop a proper solution.

@DiegoAndai DiegoAndai self-requested a review August 22, 2023 16:36
@cristianmacedo
Copy link
Contributor

cristianmacedo commented Aug 22, 2023

It appears that adding overflow: hidden to the ImageListItemRoot styled component didn't resolve the issue On my end. I suggest maybe trying a different approach:

const ImageListItemRoot = styled('li', {
... 
})(({ ownerState }) => ({
  ...
  display: 'block',
  position: 'relative',
- overflow: 'hidden',
  ...
}));

...

<ImageListItemRoot
  ...
  style={{
    height,
    gridColumnEnd: variant !== 'masonry' ? `span ${cols}` : undefined,
    gridRowEnd: variant !== 'masonry' ? `span ${rows}` : undefined,
    marginBottom: variant === 'masonry' ? gap : undefined,
+   display: variant === 'masonry' ? 'inline-block' : undefined,
    ...style,
  }}
  ...
</ImageListItemRoot>

So it prevents the immediate children of the ImageListItem component from breaking into the next column when variant === 'masonry'. This approach seems to have fixed the issue on my end, I hope it helps!

@omriklein
Copy link
Contributor Author

@DiegoAndai The overflow: hidden fixed the issue because by default, the content is not clipped to the element. When setting the overflow property to 'hidden', the content is clipped to the element.

@cristianmacedo This seems like a good solution, I'll check this out. Thanks

@omriklein
Copy link
Contributor Author

omriklein commented Aug 24, 2023

@cristianmacedo I tried your solution but the inline-block did not work correctly.
When the list has only 2 items, I got the following result:
image

I expect the images to show side by side and not one on top of the other.

@DiegoAndai
Copy link
Member

Adding the overflow: hidden property to the "masonry" variant only fixed the Woven example 👍🏼

@omriklein:

The overflow: hidden fixed the issue because by default, the content is not clipped to the element. When setting the overflow property to 'hidden', the content is clipped to the element.

I understand that. Why did clipping the content cause it to be correctly placed? Why, when the content is not clipped, it's placed in column two, but when it's clipped, it's still visible and placed in column one.

@cristianmacedo:

It appears that adding overflow: hidden to the ImageListItemRoot styled component didn't resolve the issue On my end.

Can you share an example of the code you're using that is not fixed by adding overflow: hidden. That way, we can improve the solution.

@omriklein
Copy link
Contributor Author

@DiegoAndai It seems to be a more general problem with HTML & CSS.
When creating a list with column-count > 1, items with multiple element split between the columns.
From what I could find, when the list element (li) is made out of a few elements, the elements will go to the other columns; For example, if we add another div to the list element and change column-count: 3, we can see that the 3 divs will spread across the 3 columns.

I'm yet to figure out exactly why only overflow: hidden works, but my guess is that this method of clipping the content stops it from spreading to other columns.

The most basic example of this bug is as follows:

<!DOCTYPE html>
<html lang="en">
<body>
    <ul class="list">
        <li class="item">
            <div>text 1</div>
            <div>text 2</div>
        </li>
    </ul>
</body>
<style>
    .list{
        column-count: 2;
    }
    .item{
        overflow: hidden; /* Without this line, the bug happens */
    }
</style>
</html>

@DiegoAndai
Copy link
Member

@omriklein, thanks for the minimal example of the bug. It is indeed a weird behavior.

I investigated, and #33593 introduced this issue. Changing from inline-block to block to fix #33328 introduced this one. This code sandbox example (line 12) shows that adding inline-block back fixes this issue. Of course, we cannot just do that because we would reintroduce #33328.

The root cause is that the container height is not calculated properly. This code sandbox example shows that by adding a red border to the container. What do you think about using break-inside: avoid on the item to prevent content inside it from breaking into another column? Here's a code sandbox example (line 12) of this.

@omriklein
Copy link
Contributor Author

@DiegoAndai Thank you! The break-inside: avoid did the job. I updated the code accordingly.

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @omriklein! 🎉

@DiegoAndai DiegoAndai merged commit b2c4a86 into mui:master Sep 1, 2023
18 checks passed
@oliviertassinari oliviertassinari changed the title ImageItemList fix incorrect (below) rendering [ImageItemList] Fix incorrect (below) rendering Sep 1, 2023
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 1, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 1, 2023

Nice, this PR relies on the same CSS property that I used in #36673 (comment).

@DiegoAndai I polished a bit the issue/PR titles, labels, descriptions.

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023
anon-phantom pushed a commit to anon-phantom/material-ui that referenced this pull request Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: image list This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Masonry][material] render titlebar (below) incorrectly
8 participants