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

[system][Stack] Fix displaying of rendering 0 when divider prop is passed #44126

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Oct 16, 2024

Before preview: https://mui.com/material-ui/react-stack/#basics

after preview: https://deploy-preview-44126--material-ui.netlify.app/material-ui/react-stack/#basics

Place below code in both preview to see the difference, in after preview 0 is rendered but not in before preview

import * as React from 'react';
import Box from '@mui/material/Box';
import Stack from '@mui/material/Stack';


export default function BasicStack() {
  return (
    <Box sx={{ width: '100%' }}>
      <Stack spacing={2} divider={<div />}>
        <div>Item 1</div>
        {0}
        <div>Item 2</div>
        <div>Item 3</div>
      </Stack>
    </Box>
  );
}

I was going through this issue #43969 and found logic mentioned here as bit weird as 0 would get skipped by .filter(Boolean)

@mui-bot
Copy link

mui-bot commented Oct 16, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f66f26b

@sai6855 sai6855 marked this pull request as draft October 16, 2024 12:57
@sai6855 sai6855 added bug 🐛 Something doesn't work package: system Specific to @mui/system component: Stack The React component. labels Oct 16, 2024
Comment on lines +55 to +58
if (child === 0) {
return true;
}
return !!child;
Copy link
Member

@oliviertassinari oliviertassinari Oct 16, 2024

Choose a reason for hiding this comment

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

  • Shouldn't this be using getValidReactChildren()?
  • I guess we miss
    if (isFragment(summary)) {
      return new Error('Not accepting Fragments');
    }

in getValidReactChildren()

  • It looks like Stepper, SpeedDial, Breadcrumbs, AvatarGroup should use that helper too.

Copy link
Contributor Author

@sai6855 sai6855 Oct 16, 2024

Choose a reason for hiding this comment

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

Shouldn't this be using getValidReactChildren()?

Not sure about this, React.isValidElement is being used inside getValidReactChildren and React.isValidElement is returning false when any number or strings are passed (attached reference for this below).

So if we want to directly render numbers or strings without wrapping in html tags/React components, i think these would get skipped.

image

@sai6855 sai6855 marked this pull request as ready for review October 16, 2024 17:08
@sai6855 sai6855 requested a review from mnajdova October 16, 2024 17:08
@sai6855 sai6855 changed the title [Stack] Fix displaying of rendering 0 when divider prop is passed [mui-system][Stack] Fix displaying of rendering 0 when divider prop is passed Oct 16, 2024
@sai6855 sai6855 changed the title [mui-system][Stack] Fix displaying of rendering 0 when divider prop is passed [system][Stack] Fix displaying of rendering 0 when divider prop is passed Oct 16, 2024
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: Stack The React component. package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants