-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[theme] Add border-radius to the theme #11847
Conversation
a291c8e
to
d11cb29
Compare
d11cb29
to
99f397c
Compare
@itelo Thanks for the first iteration! I have reversed the approach. We try to keep the theme object as generic as possible by default (not tight to any component). For component related customization, people can use the Following the Bootstrap approach and tailwind CSS, I think that we can expose different values by default to ease the process of building more component. Well, to be challenged: const borderRadius = {
small: 2,
medium: 4,
large: 6,
}; |
996a05d
to
d168a19
Compare
I'm not convinced of the benefit of having small, medium and large.
|
Isn't what these properties are for: using consistent values?
What's wrong with someone setting?: const theme = createMuiTheme({
borderRadius: {
small: 0,
medium: 0,
large: 0,
},
}; Ok, maybe we should be having a single value? const borderRadius = {
medium: 4,
}; or const borderRadius = {
default: 4,
}; |
@@ -49,18 +49,10 @@ export const styles = theme => ({ | |||
left: 0, | |||
right: 'auto', | |||
}, | |||
paperAnchorLeftRounded: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itelo Can we work on it in a different pull-request? Do you have a reference in the specification? I can't find it in https://material.io/design/components/sheets-side.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and we could also add it in docs. I didn't find either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliviertassinari @itelo I think shape
is not included in the docs yet.
Also, how can I set border-radius on different window sizes? Thanks.
@mbrookes I'm not sure how to handle this case. Maybe with a custom value? |
Is better, although then I wonder if it needs the Also, should it be a root key, or is there some broader category this should be grouped under? |
@oliviertassinari there are some problems if we choose to use medium, large, small.
|
@mbrookes So people can grow the list of possible border-radius value in their custom theme if they want?
@itelo The change was made to follow the specification.
I'm not sure to follow the point. What do you suggest? |
Okay, cool. Im still wondering whether it should be a root key (what if we want cornerStyle or some such at some point?), but I don't have any better suggestion. |
Should we rename |
d168a19
to
52ac531
Compare
* Add border-radius to theme * my turn :)
Add to theme defaults borderRadius and use it internally. Then we can change globally the radius of components.
Since a lot of components are based in Paper, we just need to create borderRadius.paper and borderRadius.button, following that we apply the borderRadius to:
closes #11725