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

Use @material-ui/system in the core components #15561

Closed
2 tasks done
dmtrKovalenko opened this issue May 2, 2019 · 16 comments
Closed
2 tasks done

Use @material-ui/system in the core components #15561

dmtrKovalenko opened this issue May 2, 2019 · 16 comments
Labels
new feature New feature or request priority: important This change can make a difference
Milestone

Comments

@dmtrKovalenko
Copy link
Member

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

It will be super useful to have per-breakpoint align prop. E.g. alignSm, alignLg -- them will add an ability to make responsive designs with no tears 😢

Current Behavior 😯

It is required to manually change text align for Typography
Something like that:

alignCenterSm: {
  [theme.breakpoints.down('sm')]: {
    align: 'center'
  }
}

Examples 🌈

Smth similar is already implemented in vuetify -> https://vuetifyjs.com/ru/framework/alignment

@oliviertassinari
Copy link
Member

oliviertassinari commented May 2, 2019

@dmtrKovalenko The best solution, for now, is to do:

<Box textAlign={{ xs: 'left', md: 'center' }} clone>
  <Typography />
</Box>

The long term strategy is to find a way to have the core components host the system directly so we don't have to use an extra Box + clone. So people can do:

<Typography textAlign={{ xs: 'left', md: 'center' }} />

@oliviertassinari oliviertassinari changed the title [Typography] Feature Request: responsive text align Move @material-ui/system to the core components May 2, 2019
@oliviertassinari oliviertassinari added the new feature New feature or request label May 2, 2019
@oliviertassinari oliviertassinari added this to the v5 milestone May 2, 2019
@jeremy-coleman
Copy link

jeremy-coleman commented May 8, 2019

You can use es6 proxies to add the props on demand or nest them 1 level and put them all on one prop, or use the generateId logic from glamor jss to use data- selectors and just spread into the element like <div {...boxprops}/> and just rerender style def on change

@mbrookes mbrookes changed the title Move @material-ui/system to the core components Use @material-ui/system in the core components May 9, 2019
@kevinwolfcr
Copy link

+1 for this. I don't know if it is a crazy idea, but I will suggest to wrap all components with Box. On this way, we can apply the same margin and padding properties for example on a button instead of having to wrap it with a Box.

@yordis
Copy link
Contributor

yordis commented Oct 20, 2019

I would prefer to wrap the components with Box component when it comes to positioning thing, otherwise, we will be merging concerns and create a rabbit hole where we do this with all components eventually.

I think is a good idea to separate the use Box for positioning components.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 20, 2019

and create a rabbit hole where we do this with all components eventually.

@yordis Yes, it's the idea. There is a related opportunity we can explore. We could have a component that generates thousands of global class names, a là tailwind, that we could apply directly as class names. It would extend the system support to all elements on the page.

<button className="p-3" />

@yordis
Copy link
Contributor

yordis commented Oct 20, 2019

I think that tailwind is targeting different methodologies on how we design and develop applications in my opinion, the more you rely on something being there (global classes) the harder it is to move code since you trade-off cohesion.

I love the idea, but I would push it into the @material-ui/system rather than Mui.

@andyfangaf
Copy link

andyfangaf commented Apr 21, 2020

@oliviertassinari is this going to be added for v5? Or is there a canary version where that's already available?

Changing the font-weight of <Typography/> is super important if you want responsive font sizes. If you use any of the hacks like referencing the theme.fontSize or initial, you're stuck with the smallest screen font size, which isn't adaptive.

Right now, there's no way to have responsive font sizes for typography with different font weights than what's specified in the theme. This is a very noticeable inconsistency if you're already using responsiveFontSizes.

@giovanniantonaccio

This comment has been minimized.

@matthewwolfe
Copy link

@oliviertassinari I see mentioned above applying system to the core components, but I am also curious about applying breakpoint-esque functionality to non-system props. It would be amazing to be able to have something like handleBreakpoints from @material-ui/system for many more props.

For example, AppBar elevation prop currently accepts a number, but it would be cool to modify any props like that at different breakpoints:

<AppBar elevation={{xs: 1, md: 2, lg: 4}}>...</AppBar>

Is that something that would be considered?

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 15, 2020

@matthewwolfe Regarding the elevation, I think that we can consider it as part of the system, and is already in https://material-ui.com/system/shadows/#example. We will need to remove duplication.

However, if we are pushing in the {xs: 1, md: 2, lg: 4} API direction, I do agree that it would make sense to expose something for the props that don't support the breaking object.

I wonder if we shouldn't have a:

What do you think about it?

@stunaz
Copy link
Contributor

stunaz commented Aug 16, 2020

useBreakpoint ()

@eps1lon
Copy link
Member

eps1lon commented Oct 26, 2020

Why do we need this for all components? Just because it might make sense for one component doesn't mean it makes sense for all of them. As far as I can tell it'll be a footgun for widgets since it leads down a blind alley: you start styling with sx and later realize it's inssufficient since you can't target child components. Even more problematic: With each API we increase the risk of duplicate styling which was one of the problems CSS-in-JS tried to solve. We're now moving backwards in that regard.

I would understand if this would be replaced at compile time with the styling engine but it doesn't make sense to add another runtime API that does the same thing but different. This means that either API is insufficient and overcoming it by adding both just adds technical debt we have to pay long term.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 26, 2020

you can't target child components

@eps1lon we can target child components with CSS selectors in the sx prop, but it's probably to be avoided. I think that it's best to only style the element it's applied on.

the risk of duplicate styling

I don't understand, what duplication are you referring to? @mnajdova is planning to add a section in the documentation about when using the sx vs styled API. We should probably explain the pros and cons too, the why.

Why do we need this for all components?

For the same reason it's useful on a Box. Why does Bootstrap has CSS utils? I think that the best case is when we can use it on every DOM node mounted. It's where it get its true power. I think that there is nothing simpler, easier to understand, and faster that to declaratively set the style on the element.

it doesn't make sense to add another runtime API that does the same thing but different

Are you saying sx and styled are solving the same problems? We definitely need to document the differences, why this is important to have both and the best use cases for each. Emotion has the two, styled-components has the two too.


We might be able to close, we could argue that the issue was solved with #23205. We are also exploring how to extend the support to all primitives in #23220.

@eps1lon
Copy link
Member

eps1lon commented Oct 26, 2020

we can target child components with CSS selectors in the sx prop

So we don't even want people to use styled then?

I don't understand, what duplication are you referring to?

Duplication of styles in sx and styled decoration.

For the same reason it's useful on a Box.

The Box and Slider are by defition not the same component. It does not follow that if X makes sense for A that X makes also sense for B. Otherwise you're arguing that it makes sense to have an exhaust on your smartphone because it makes sense to have it on your car.

@mnajdova is planning to add a section in the documentation about when using the sx vs styled API. We should probably explain the pros and cons too, the why.

This needs to be explored first though. We can't just implement things and then look at the API we created.

Emotion has the two, styled-components has the two too.

Both APIs are used on primitives first and not composite components. It also still doesn't follow that if A uses X that B should use X. You're arguing that we should use Flash because there are some sites that are still using flash. We go by requirements not existence.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 26, 2020

This needs to be explored first though.

@eps1lon Agree, I believe we have tried to cover it in the pull requests/issues/blog post, but we can improve here. For the "why?", there are 4 written content listed in https://trello.com/c/IUve1J6o/1577-system-core-component that are on the topic. I think that the timing is perfect to cover the documentation side of things in more detail. We have had a question on the same subject from a template author on the store recently.

Maybe we can first get to a plan of action here and then convert it to the documentation, then close this issue? cc @mnajdova

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 27, 2021

I'm closing for #24405. Each migrated component gets the feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests

10 participants