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

[Layout] Add a new component #5808

Merged
merged 1 commit into from
Dec 25, 2016
Merged

[Layout] Add a new component #5808

merged 1 commit into from
Dec 25, 2016

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Dec 20, 2016

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).
  • Document the properties
  • Write some unit tests
  • Add some regressions tests

This is a port of the layout component we use at @doctolib.
We can see that effort as an alternative to #5267 that might or not succeed.
I have made some important API / implementation tradeoffs.

  • We do not rely on the matchMedia API to be fully server side rendering compliant. Each xs, sm, md and lg variations increase the size of the style injected in the DOM.
  • It's using a 12 grids system as suggested in the specification. Instead of a more granular flex property.
  • It's exposing a single component that can be an item or a container. There is no <Col /> nor <Row /> components.

dec -20-2016 01-02-12

Closes #3614

@oliviertassinari oliviertassinari changed the title [WIP][LayoutBloc] Add a new component [WIP][LayoutBlock] Add a new component Dec 20, 2016
@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Dec 20, 2016
@oliviertassinari oliviertassinari changed the title [WIP][LayoutBlock] Add a new component [LayoutBlock] Add a new component Dec 21, 2016
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 21, 2016
import classNames from 'classnames';

const GUTTERS = [8, 16, 24, 40];
const GIRD_SIZES = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12];
Copy link
Member

Choose a reason for hiding this comment

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

Spelling

@rosskevin
Copy link
Member

Looks good. Without using it my only concern is reducing verbosity where possible.

For example:

<LayoutBlock type="item" xsGrid={6} smGrid={3}>

I think this would be better written as:

<Layout xs={6} sm={3}>

Details:

  • Block adds no value as a description to the component - Layout is sufficient
  • *Grid The primary used props are for the responsive grid sizes - all labeled *Grid. Whereas I totally understand that there are other grid based props e.g. xsDirection, I feel that this one could really be just xs, sm, md and leaving xsDirection etc as-is
  • type could be defaulted as item as it will be the most common use

I think these would be good changes and will not reduce clarity given the frequency of use.

@oliviertassinari
Copy link
Member Author

@rosskevin Thanks for the feedback!

type could be defaulted as item as it will be the most common use

I completely agree.

*Grid The primary used props are for the responsive grid sizes - all labeled *Grid.

That's a good idea.

Block adds no value as a description to the component - Layout is sufficient

You raised a good point. I wasn't happy with that LayoutBlock name.
I have some other ideas:

  • LayoutGrid
  • Grid

Bootstrap just dropped the float implementation of the layout. I'm gonna see there if I can't reuse some good part: http://v4-alpha.getbootstrap.com/layout/flexbox-grid/

@oliviertassinari oliviertassinari changed the title [LayoutBlock] Add a new component [Layout] Add a new component Dec 21, 2016
@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Dec 21, 2016
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 21, 2016
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 21, 2016

@rosskevin I have updated the PR with your feedbacks.
That Layout component can be improved in many ways. For instance, by:

  • Writing more tests
  • Writing more documentation, bootstrap have much more
  • Adding more flex properties like xsOrder or md variations.

I have tried to keep things simple. We should be able to continue the effort in a second run once we feel the needs for it.

@stunaz
Copy link
Contributor

stunaz commented Dec 23, 2016

Hi @oliviertassinari, really nice start, here are a few questions/comment :

  • Why there is a type of Layout (item or container) - I think it should not exist a type, a block may be able to content block without having end user specifying the type
  • I dont understand the role/need of the xs attribute as a boolean value i.e in the Auto-Layout example, does this mean that they will be an sm, md, lg boolean attributes too ? AngularMaterial just use the flex attributes for that.
  • The docs should indicate the breakpoints that you are using for xs, sm=600px? , md=960px?, lg=1200 ? ...
  • I see props like xsGutter, xsDirection, xsJustify, xsAlign, I hope to also see smGutter, mdGutter, smDirection ... to have different comportement applied based on the breakpoint.
  • Apppart for properties like xsOrder or md variations as you mentioned, it will be nice to see others like xsVisible, smVisible ... or xsHide, smHide ....

@PhilipGarnero
Copy link

PhilipGarnero commented Dec 23, 2016

Good work @oliviertassinari !

I have one small question : why is the layout component not split into two distinct components ? Is there any practical reason the container and the item version should live together ? I didn't notice props that were used by the two components in your examples.
It is just a matter of tastes, but rather than a type attribute, I'd prefer two distinct components. Unless of course, you have a technical reason for them to be under the same component.

Anyways, thank you for your work 👍

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 23, 2016

Why there is a type of Layout (item or container)

why is the layout component not split into two distinct components?

That's ambivalence is at the heart of the CSS’s flexbox model. Just reading the first line of the specification
You gonna find:

In the flex layout model, the children of a flex container.

So why not having two separate components?
Because you might want to have a <Layout /> behaving as a container and an item at the same time.
That was my original implementation but ultimately, I reverted as I had gutter issues and thought it would be better to be closer to the bootstrap model, i.e. simpler.

I dont understand the role/need of the xs attribute as a boolean value

That's basically adding flex-grow: 1. So you can get shared screen width without having to think about the right xs value (12 / number_of_items).

The docs should indicate the breakpoints that you are using

I have updated the documentation

I hope to also see smGutter, mdGutter, smDirection ... to have different compartment applied based on the breakpoint.

Yes, that xs prefix was added to anticipate that need. I don't want to implement it for now. But the API / implementation is allowing us to grow into that direction.

Apppart for properties like xsOrder or md variations as you mentioned, it will be nice to see others like xsVisible, smVisible ... or xsHide, smHide ....

I agree, as the previous answer. I want to keep thing simple to begin with.

@stunaz
Copy link
Contributor

stunaz commented Dec 23, 2016

That's basically adding flex-grow: 1. So you can get shared screen width without having to think about the right xs value (12 / x).

I now get it, xs applied to all screens size, I first thought it was a breakpoint, for screens smaller than sm

});
});

describe('prop: component', () => {
Copy link
Member

Choose a reason for hiding this comment

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

prop: xs?

});

describe('prop: component', () => {
it('should change the component', () => {
Copy link
Member

Choose a reason for hiding this comment

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

We should also test spreading props properly to the component.

@nathanmarks
Copy link
Member

nathanmarks commented Dec 25, 2016

@oliviertassinari What are your thoughts RE the amount of styles that would be pre-created if sm, md, lg variants were added for various options? Non-issue? I think users will ask for this. Or at least one more set.

Overall looking really awesome.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 25, 2016

hat are your thoughts RE the amount of styles that would be pre-created

@nathanmarks Thanks for the review.
We have 5 properties missing the variations. That's gonna make 25 more CSS properties injected in the DOM. I don't feel like it's much, but that just an intuition, I would need to benchmark that.

Regarding other set of properties that people might want to add, I see some candidates:

  • offset
  • padding
  • margin
  • order
  • hide: I would keep that into a different component
  • show: I would keep that into a different component

Or politic could be to let users implement the style in their side until we have a way to lazy inject the style or some benchmark showing us it's fine.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 25, 2016

Let's move forward. I'm gonna merge that PR. I'm quite happy with the API / implementation.
We will see how the community respond when trying it out.

@oliviertassinari
Copy link
Member Author

Thanks everybody for your feedbacks!

@oliviertassinari oliviertassinari merged commit 692e74c into mui:next Dec 25, 2016
@oliviertassinari oliviertassinari deleted the layout-block branch December 25, 2016 21:23
@gaguirre
Copy link

I tried with npm install material-ui@next but Layout is not there.
From where should I get the next version? @oliviertassinari

@mbrookes
Copy link
Member

mbrookes commented Dec 29, 2016

@gaguirre It isn't published yet, but you can find the code here: https://github.com/callemall/material-ui/tree/next

@gaguirre
Copy link

Thanks @mbrookes, do you have plans to publish a new version soon?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 29, 2016

@nathanmarks might be working on automating releases of beta version of the next branch on a fix period basis.

@gaguirre
Copy link

Great! Thanks @oliviertassinari @mbrookes

@Extra-lightwill
Copy link

Extra-lightwill commented Jan 5, 2017

Hi @oliviertassinari I've been testing out this layout component and it’s really great - however, i’m just wondering how you would reference the xsAlign, xsDirection, xsJustify and xsWrap props (I can’t seem to figure it out and for me, it's not clear from your commits in the PR.) Including it like below, throws an error:

 <Layout type="item" 
          xs={6}
          xsJustify="center"
          xsAlign="center"
          >

ReferenceError: xsJustify is not defined

I've been looking for a comprehensive component based layout solution with React/flexbox and stumbled upon this - although it's ofc quite new, it seems to really fit the bill and would love to use it. Thanks.

@oliviertassinari
Copy link
Member Author

@Extra-lightwill Please open an issue. Have a look at that docs regarding how to use it https://material-ui-1dab0.firebaseapp.com/#/layout/responsive-ui.

@RohovDmytro
Copy link

RohovDmytro commented Jan 12, 2017

How about treating xs={true} as xsVisible={true}? And so on.

@oliviertassinari
Copy link
Member Author

@rogovdm I'm wondering if xsVisible should be handled at the Layout level. That sounds like a generic helper. I think that we could provide it with a Higer-order Component or a wrapper component specialized in that.

@RohovDmytro
Copy link

RohovDmytro commented Jan 12, 2017

@oliviertassinari maybe, but I'm not sure. I use react-grid-system components and it become quite annoying to wrap thing with additional Hidden component all over the place. My pros is that grid building is tightly coupled with hiding stuff based on xs, sm, lg etc.

Will there be much overhead if Layout will be used is some cases only for hiding or making visible?

@oliviertassinari
Copy link
Member Author

@rogovdm I get your tradeoff. I think we should wait having more feedbacks.

@jaredkwright
Copy link

I am starting a greenfields project using the next branch and I am currently looking for a good way to hide/render components based on screen size. For example, I might want to use a simplified logo on mobile but the full logo on desktop. Does Layout currently support a way of doing this, or am I thinking about the problem wrong?

@oliviertassinari
Copy link
Member Author

Does Layout currently support a way of doing this

@jaredkwright No, he doesn't. I think that we could be providing a component / Higher-order Component for that use case.

@mikeatm
Copy link

mikeatm commented Feb 24, 2017

@oliviertassinari looks quite neat, is there a plan to backport it to <1.0.0 release? I tried the next branch, but its still missing a bunch of components (or better yet is there an ETA on 1.0.0 with layout and all components?)

@oliviertassinari
Copy link
Member Author

looks quite neat, is there a plan to backport it to

@mikeatm Thanks. No, we don't have the resources to maintained two versions same, we don't have the resource to give a ETA.

@advance512
Copy link

I also attempted using it with the existing release - no dice, there is no styleManager in the existing release's MuiThemeProvider, but it is required to generate styles in the Layout class.

Using two MuiThemeProviders from the existing release and next release, together, seems a bit unhealthy, not to mention it's quite hacky getting both existing and next working together.

Is there any recommendation for a Layout that can be used /today/ with Material UI to allow automatic layouts, similar to the Material.io layouts?

@mikeatm
Copy link

mikeatm commented Mar 16, 2017

ran into this today, https://jsxmachina.github.io/react-grid-system/ , no idea if works, yet to test it with material-ui

@damianobarbati
Copy link

Yet another essential/cool thing... just in the next branch :(
Is it possible to use this in current material-ui version?

Great work as always guys!

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 31, 2017

@damianobarbati It's published under the 1.0.0-alpha.x version. You can play with it, today, (docs).

@damianobarbati
Copy link

@oliviertassinari thanks so much for the quick answer, trying right now! Is there a documentation referring to the branch? I see lots has changed in components importing structure (icon, button) and theme (getMuiTheme not defined) and so on.
Thanks!

@oliviertassinari
Copy link
Member Author

I have linked the documentation in the previous comment. It's still work in progress.

@zannager zannager added the docs Improvements or additions to the documentation label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Responsive Material-UI