search

geist-org / geist-ui

A design system for building modern websites and applications.
https://geist-ui.dev
MIT License
4.33k stars 334 forks source link

Grid with hidden on xs keeps the higher sizes hidden when specified #441

Closed amine605 closed 3 years ago

amine605 commented 3 years ago

Bug report 🐞

Version & Environment

Expection

When adding a grid item with both xs={0} and sm={24}. the item should be hidden on xs, and visible on sm, md, lg, xl

Actual results (or Errors)

The item remains hidden even on sm, md, lg and xl.

amine605 commented 3 years ago

I did some investigation, and I found the cause of the unexpected behaviour. in case the xs value is 0, the display: none will be added to the class .xs and not only to the media query which results in a hidden component even if higher sizes are specified . I'm opening a pull request for it.

unix commented 3 years ago

Sorry for the late reply. This behavior is in line with expectations.

In a responsive design system, we must specify a default value for each type of width (xs/sm/md/lg/xl). If the component lacks this default value, Geist UI will not be able to determine what to do with it.

As you can see in the component documentation, xs refers not only to 0 - 650px, xs also contains widths greater than 650px. This means that in any scenario where you set the xs={24}, the breakpoint will be demoted to xs={24} when it is higher than the width of xs.minWitdh (0px).

This is a bit strange, see the following example:

1. I need to set the width ratio of User1 and User2 at any breakpoint to 1:1, so with the current design, it is only necessary to set xs:

<Grid xs={12}><User1 /></Grid>
<Grid xs={12}><User2 /></Grid>

2. Assuming that xs refers to just 0 - 650px, then I need to repeat the value at each breakpoint: (If I keep all breakpoints working properly, then I always need to set 5 values)

<Grid xs={12} sm={12} md={12} lg={12} xl={12}><User1 /></Grid>
<Grid xs={12} sm={12} md={12} lg={12} xl={12}><User2 /></Grid>

Most users don't always consider all breakpoints in their scenarios, they only need to set a few minimum width breakpoints and the component will work fine in all scenarios. Perhaps we can understand breakpoints in this way:

xs: 0 - 650, 650 - max,
sm: 650 - 900, 900 - max,
md: 900 - 1280, 1280 - max,
lg: 1280 - 1920, 1920 - max,
xl: 1920 - max,

Suppose my page only needs to consider using different layouts for mobile and desktop, and I want to use the desktop layout for all widths greater than 650px, then, this is all I need:

<Grid.Container gap={2} justify="center">
    <Grid xs sm={24}><Header ></Grid>
    <Grid xs sm={24}><Body /></Grid>
</Grid.Container>

Now Header and Body each occupy one row of space on the desktop and are in the same row on the mobile. Then one day I need to compatible with Pad, so I just need to make the following changes:

<Grid.Container gap={2} justify="center">
    <Grid xs sm={16} md={24}><Header ></Grid>
    <Grid xs sm={8} md={24}><Body /></Grid>
</Grid.Container>

This design allows most users to write less code, which is an important part of what I considered when designing this component. And any different ideas are welcome and I'd like to hear what you think of this.

amine605 commented 3 years ago

@unix

Thanks for the detailed answer. yet, I think you misunderstood the bug that I'm addressing here. this issue appears only when putting <Grid xs={0} sm={24} > <Component /> </Grid> {/* { can change sm with any size other than xs} */} the expected behavior xs: component hidden sm component visible Currently, xs: component hidden sm: still hidden.

btw, same for <Grid sm={0} md={24} > <Component /> </Grid> Whenever there's a size={0} there's the unexpected behaviour.

you can take a look on the PR that fixes this issue: #442 The issue is simple when specifying x={0} a display: none style will be added. and with it when we specify sm={24} there should be a display: unset style added in that breakpoint

unix commented 3 years ago

You are right, I understand your intention now. I've updated the PR, you can review it when you have time. @amine605