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

[Masonry] Initialized with no children results in error #29362

Closed
2 tasks done
joziahg opened this issue Oct 28, 2021 · 8 comments · Fixed by #29452
Closed
2 tasks done

[Masonry] Initialized with no children results in error #29362

joziahg opened this issue Oct 28, 2021 · 8 comments · Fixed by #29452
Assignees
Labels
bug 🐛 Something doesn't work component: masonry This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@joziahg
Copy link

joziahg commented Oct 28, 2021

Initializing the Masonry component with no children or an empty array of children results in an error.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When Masonry is initialized with no children or an empty of children, the component errors.

Expected Behavior 🤔

Masonry errors if initialized with no children. Setting children to an empty array after initialization results in no error.

Steps to Reproduce 🕹

Steps:
https://codesandbox.io/s/basicmasonry-material-demo-forked-thsv4?file=/demo.js

  1. Click Remove Items Button => no Error
  2. Remove numbers in heights array & refresh => Error

Context 🔦

Your Environment 🌎

`npx @mui/envinfo`
  System:
    OS: Linux 4.19 Ubuntu 18.04.5 LTS (Bionic Beaver)
  Binaries:
    Node: 14.17.0 - ~/.nvm/versions/node/v14.17.0/bin/node
    Yarn: 1.22.5 - ~/.yarn/bin/yarn
    npm: 6.14.13 - ~/.nvm/versions/node/v14.17.0/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: 93.0
  npmPackages:
    @emotion/react: ^11.5.0 => 11.5.0 
    @emotion/styled: ^11.3.0 => 11.3.0 
    @mui/core:  5.0.0-alpha.53 
    @mui/icons-material: ^5.0.5 => 5.0.5 
    @mui/lab: ^5.0.0-alpha.53 => 5.0.0-alpha.53 +  https://github.com/mui-org/material-ui/pull/29351
    @mui/material: ^5.0.6 => 5.0.6 
    @mui/private-theming:  5.0.1 
    @mui/styled-engine:  5.0.2 
    @mui/styles: ^5.0.2 => 5.0.2 
    @mui/system:  5.0.6 
    @mui/types:  7.0.0 
    @mui/utils:  5.0.1 
    @mui/x-data-grid-pro: ^5.0.0-beta.2 => 5.0.0-beta.2 
    @mui/x-license-pro:  5.0.0-beta.2 
    @types/react:  17.0.24 
    react: ^17.0.2 => 17.0.2 
    react-dom: ^17.0.2 => 17.0.2 
@joziahg joziahg added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 28, 2021
@mnajdova mnajdova added bug 🐛 Something doesn't work component: masonry This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 29, 2021
@hbjORbj
Copy link
Member

hbjORbj commented Oct 29, 2021

@joziahg Thanks for the report! The following change will do:

diff --git a/packages/mui-lab/src/Masonry/Masonry.js b/packages/mui-lab/src/Masonry/Masonry.js
index 78c23bf31c..c47f92d19b 100644
--- a/packages/mui-lab/src/Masonry/Masonry.js
+++ b/packages/mui-lab/src/Masonry/Masonry.js
@@ -180,6 +180,9 @@ const Masonry = React.forwardRef(function Masonry(inProps, ref) {
 
   React.useEffect(() => {
     const handleResize = () => {
+      if (!masonryRef.current.firstChild) {
+        return;
+      }
       const parentWidth = masonryRef.current.clientWidth;
       const childWidth = masonryRef.current.firstChild.clientWidth;
       const firstChildComputedStyle = window.getComputedStyle(masonryRef.current.firstChild);

Would you like to create a PR for this 😃?

@hbjORbj hbjORbj added the good first issue Great for first contributions. Enable to learn the contribution process. label Oct 29, 2021
@joziahg
Copy link
Author

joziahg commented Oct 29, 2021

@hbjORbj Sure, seems like an easy first PR

@abdelrazek-alaa
Copy link

I'm a complete newbie.
ofc, it would be amazing, guys :)

@abdelrazek-alaa
Copy link

@joziahg I'm so sorry I didn't notice that you intend to do a PR. I'm not an English speaker so I didn't get it.
what should I do now?. how could I undo what I've made.
our maintainers please do what is right for this situation, and if @joziahg is still wanna do PR please delete my own PR.
I'm so sorry guys.

@joziahg
Copy link
Author

joziahg commented Oct 29, 2021 via email

@abdelrazek-alaa
Copy link

@joziahg thank you for your kindness.

@hbjORbj
Copy link
Member

hbjORbj commented Oct 30, 2021

@abdelrazek-alaa @joziahg Thank you both :)

@hbjORbj
Copy link
Member

hbjORbj commented Oct 30, 2021

@joziahg Would you still like to create a PR? For testing, please go to /test/regressions/fixtures/Masonry and create a file EmptyMasonry.js with the following code.

import * as React from 'react';
import Masonry from '@mui/lab/Masonry';

export default function EmptyMasonry() {
  return <Masonry columns={3} spacing={1} />;
}

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: masonry This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
4 participants