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

[Grid] API sugar proposal #10343

Closed
RohovDmytro opened this issue Feb 19, 2018 · 7 comments
Closed

[Grid] API sugar proposal #10343

RohovDmytro opened this issue Feb 19, 2018 · 7 comments

Comments

@RohovDmytro
Copy link

Grid component has quite a few props. And those are great. A person (or a group) who came out with this API are awesome 👍

I have a small proposal. In order to group properties visually (because it'a lot of them) I came up with this idea:

image

Nice, but why not proposing adding a sizes prop?

To be clear. The benefit is to have them grouped. When it's time to do some adaptive stuff there is a lot going on with those values.

image

@oliviertassinari
Copy link
Member

@rogovdm This issue is deeply linked to #6140. I was suggesting doing it the other way around:

<Grid
  xs={12}
  xsGrow={1}
  xsDirection={column}
  lg={6}
  hidden={{ xlUp: true }}
>

or

<Grid
  xs={{
    size: 12,
    grow: 1,
    direction: column,
  }}
  lg={6}
  hidden={{ xlUp: true }}
>

But I'm not sold on any specific API. It's something we can start thinking about for Material-UI v2.

@yordis
Copy link
Contributor

yordis commented Mar 27, 2018

@oliviertassinari I would target for an API that it is flexible to changes.

If you go for xsGrow={} then every single breakpoint will have that and from code perspective you are not longer dealing a fixed attributes on the component, no counting that for x given attribute (hidden) you have breakpoint length of places where you need to look up.

The second approach at least it is clear what is it doing, for x given attribute I could expect an object where the key,value pair represent the breakpoint and the value of the attribute on that breakpoint.

From code perspective, you dont need to pay that much attention on the breakpoints as long as you had configured the values somewhere (either by configuration the theme or a single object on the code).

@itelo
Copy link
Contributor

itelo commented Aug 3, 2018

Thinking on enhancement API for Material-UI v1 that's not break anything...
What about put breakpoints in Grid container, for example:
(this is heavily inspired in angular material v1 - layout)

<Grid
  container
  xsDirection="column"
  mdDirection="row"
  xsAlignItems="center"
  mdAlignItems="baseline"
  {...othersProps}
>
/* code here*/
</Grid>

them, to not be a breaking change, we could assume that if the breakpoint is not present, the default will be used (xs), like we do with Grid item, example:
direction became xsDirection
alignItems became xsAlignItems
justify became xsJustify
alignContent became xsAlignContent

Today, I can accomplish this using withWidth, them my code became this: (no judge here, pls xD)

<Grid
  container
  direction={width === "xs" ? "column" : "row"}
  alignItems={width === "xs" ? "center" : "baseline"}
  {...othersProps}
>
/* code here*/
</Grid>

@oliviertassinari
Copy link
Member

@itelo I'm happy to push this story forward once we have #7633. It should unlock of lot of new features. We can rethink the Grid API. I'm very eager to benchmark what the other libraires have been doing since then. Right now, we are limited by the CSS generated output.

@yordis
Copy link
Contributor

yordis commented Aug 12, 2018

After reasoning about this I think the best approach is to minimize the amount of CSS we need for this and by so the bundle size decrease and maybe is not that bad to use style attributes or classes by without the breakpoint scoping since it wouldn't matter. Something really similar to what @itelo shared.

With a declarative component and some helper function that makes it easier and friendly to read we could use matchMedia API.

<MatchBreakpoint>
  (breakpoint)=> {
    <Grid
       container
       direction={breakpoint === "xs" ? "column" : "row"}
       alignItems={breakpoint === "xs" ? "center" : "baseline"}
       {...othersProps}
     ></Grid>
  }
</MatchBreakpoint>

The more I think about this the more I like this approach, the only concern it would be the performance of matchMedia and so on (hopefully nobody do premature opinions without data because I dont have data to back it up) but I dont believe it should be an issue.

@oliviertassinari In any case, I would love to push MatchBreakpoint component to Mui since I would probably use it for other things and I like the declarative approach driven by JS.

Would you take a PR for the component?

@yordis
Copy link
Contributor

yordis commented Aug 12, 2018

Btw, it is kind of like WithWidth but

  • No HOC
  • No resizing (expensive)

@oliviertassinari
Copy link
Member

I'm closing for #6140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants