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.

Sign up for GitHub

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

Jump to bottom

[Slider] Add system prop #22819

Closed
wants to merge 19 commits into from
Closed

[Slider] Add system prop #22819

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c2d31cc
add emotion peer dependencies
mnajdova Sep 25, 2020
5ae933f
fixed types & tests
mnajdova Sep 25, 2020
18b0668
prettier
mnajdova Sep 25, 2020
f0ef95c
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 28, 2020
c7bebb8
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 28, 2020
92b2d6e
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 28, 2020
13da531
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 29, 2020
cf5d9a5
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 29, 2020
b8d1291
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 30, 2020
66c1656
wip
mnajdova Sep 30, 2020
de39dd6
removed console.logs
mnajdova Sep 30, 2020
830d99a
moved systemPropsFunction to muiStyled
mnajdova Sep 30, 2020
4f8aacb
added typings
mnajdova Sep 30, 2020
8967a49
prettier
mnajdova Sep 30, 2020
2f36b4c
refactorings
mnajdova Sep 30, 2020
a980971
fix
mnajdova Sep 30, 2020
621bdd8
import order
mnajdova Sep 30, 2020
d906feb
changed description
mnajdova Sep 30, 2020
0822e62
renamed useSystemProps to addSystemProps
mnajdova Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/pages/api-docs/slider-styled.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">orientation</span> | <span class="prop-type">'horizontal'<br>&#124;&nbsp;'vertical'</span> | | The slider orientation. |
| <span class="prop-name">scale</span> | <span class="prop-type">func</span> | | A transformation function, to change the scale of the slider. |
| <span class="prop-name">step</span> | <span class="prop-type">number</span> | | The granularity with which the slider can step through values. (A "discrete" slider.) The `min` prop serves as the origin for the valid values. We recommend (max - min) to be evenly divisible by the step.<br>When step is `null`, the thumb can only be slid onto marks provided with the `marks` prop. |
| <span class="prop-name">system</span> | <span class="prop-type">object</span> | | System props to apply to the component. |
| <span class="prop-name">track</span> | <span class="prop-type">'inverted'<br>&#124;&nbsp;'normal'<br>&#124;&nbsp;false</span> | | The track presentation:<br>- `normal` the track will render a bar representing the slider value. - `inverted` the track will render a bar representing the remaining slider value. - `false` the track will render without a bar. |
| <span class="prop-name">value</span> | <span class="prop-type">Array&lt;number&gt;<br>&#124;&nbsp;number</span> | | The value of the slider. For ranged sliders, provide an array with two values. |
| <span class="prop-name">valueLabelDisplay</span> | <span class="prop-type">'auto'<br>&#124;&nbsp;'off'<br>&#124;&nbsp;'on'</span> | | Controls when the value label is displayed:<br>- `auto` the value label will display when the thumb is hovered or focused. - `on` will display persistently. - `off` will never display. |
Expand Down
1 change: 1 addition & 0 deletions docs/pages/api-docs/slider-unstyled.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">orientation</span> | <span class="prop-type">'horizontal'<br>&#124;&nbsp;'vertical'</span> | <span class="prop-default">'horizontal'</span> | The slider orientation. |
| <span class="prop-name">scale</span> | <span class="prop-type">func</span> | <span class="prop-default">(x) => x</span> | A transformation function, to change the scale of the slider. |
| <span class="prop-name">step</span> | <span class="prop-type">number</span> | <span class="prop-default">1</span> | The granularity with which the slider can step through values. (A "discrete" slider.) The `min` prop serves as the origin for the valid values. We recommend (max - min) to be evenly divisible by the step.<br>When step is `null`, the thumb can only be slid onto marks provided with the `marks` prop. |
| <span class="prop-name">system</span> | <span class="prop-type">object</span> | | System props to apply to the component. |
| <span class="prop-name">track</span> | <span class="prop-type">'inverted'<br>&#124;&nbsp;'normal'<br>&#124;&nbsp;false</span> | <span class="prop-default">'normal'</span> | The track presentation:<br>- `normal` the track will render a bar representing the slider value. - `inverted` the track will render a bar representing the remaining slider value. - `false` the track will render without a bar. |
| <span class="prop-name">value</span> | <span class="prop-type">Array&lt;number&gt;<br>&#124;&nbsp;number</span> | | The value of the slider. For ranged sliders, provide an array with two values. |
| <span class="prop-name">valueLabelDisplay</span> | <span class="prop-type">'auto'<br>&#124;&nbsp;'off'<br>&#124;&nbsp;'on'</span> | <span class="prop-default">'off'</span> | Controls when the value label is displayed:<br>- `auto` the value label will display when the thumb is hovered or focused. - `on` will display persistently. - `off` will never display. |
Expand Down
1 change: 1 addition & 0 deletions docs/src/pages/components/slider-styled/ContinuousSlider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export default function ContinuousSlider() {
</Grid>
<Grid item xs>
<Slider
system={{ color: 'secondary.main' }}
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes will be reverted before merging

value={value}
onChange={handleChange}
aria-labelledby="continuous-slider"
Expand Down
1 change: 1 addition & 0 deletions docs/src/pages/components/slider-styled/ContinuousSlider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export default function ContinuousSlider() {
</Grid>
<Grid item xs>
<Slider
system={{ color: 'secondary.main' }}
value={value}
onChange={handleChange}
aria-labelledby="continuous-slider"
Expand Down
6 changes: 5 additions & 1 deletion packages/material-ui-lab/src/SliderStyled/SliderStyled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const overridesResolver = (props, styles, name) => {
const SliderRoot = muiStyled(
'span',
{},
{ muiName: 'MuiSlider', overridesResolver },
{ muiName: 'MuiSlider', overridesResolver, useSystemProps: true },
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
)((props) => ({
height: 2,
width: '100%',
Expand Down Expand Up @@ -436,6 +436,10 @@ Slider.propTypes = {
* @default 1
*/
step: PropTypes.number,
/**
* System props to apply to the component.
*/
system: PropTypes.object,
Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably come up with better name here... But I would definitely vote for defining all system props under one prop instead of spreading them on each component, mainly because of the following reasons:

  • we are already specifying all styles props under one prop - stylesProps
  • we would avoid props explosion
  • all system props would be easily discoverable

Copy link
Member

Choose a reason for hiding this comment

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

systemProps for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking more of a prop like styles, or css (css will conflict with emotion's css), but even systemProps sounds better :)

Copy link
Member

Choose a reason for hiding this comment

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

Let's get some other feedback before changing it.

Copy link
Member

@oliviertassinari oliviertassinari Sep 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should we have a 📊 on Twitter about the best API for the system? styled-system pushes for flattening; theme-ui.com pushes for isolation under one prop, well they have props too. Both have the same author.

A benchmark: https://trello.com/c/IUve1J6o/1577-system-core-component

Copy link
Member

@eps1lon eps1lon Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

and well known

We need to be careful with that statement and consider audiences. A very large portion have a backend background. So far I've only heard of it in the context of Theme-UI.

Copy link
Member

Choose a reason for hiding this comment

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

So system > sx > css?

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the flatten vs prop tradeoff. Any thoughts on it? The summarized version:

  • Box component: subset of CSS, flattened system props + superset of CSS, system prop
  • Most core components: superset of CSS, system prop only
  • Some core components that are responsible for accessing design tokens or doing layout have flattened system prop: Grid, Stack, Typography (e.g. fontWeight).

Copy link
Member Author

Choose a reason for hiding this comment

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

So system > sx > css?

Yep, and I am saying system in case if we are going to use it as a simply collection of system props, if we are going to treat is as a superset of CSS, then I would rather use a prop like styles or sx for it... In my opinion there is a big difference between these two use-cases

Regarding the flatten vs prop tradeoff. Any thoughts on it? The summarized version:

Box component: subset of CSS, flattened system props + superset of CSS, system prop

Agree, again if we are using the system props as superset of CSS I am revoking the system prop as a name, as it is not system anymore is more styles/css. 😄

Most core components: superset of CSS, system prop only

Agree 👍

Some core components that are responsible for accessing design tokens or doing layout have flattened system prop: Grid, Stack, Typography (e.g. fontWeight).

These should basically be the same as Box then, both flattened system prop + sx or styles. If that is the case I agree, and we should make sure we document all these components under the same section, so it would make sense to people why some components have them flattened and other don't...

Copy link
Member

Choose a reason for hiding this comment

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

Yep, and I am saying system in case if we are going to use it as a simply collection of system props, if we are going to treat is as a superset of CSS

From my perspective, we should go with the superset of CSS for the standalone property. We would gain an improved version of the CSS prop without the limitations of the Box. I can't think of any real downside.

/**
* The track presentation:
*
Expand Down
5 changes: 5 additions & 0 deletions packages/material-ui-lab/src/SliderUnstyled/SliderUnstyled.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { OverridableComponent } from '@material-ui/core/OverridableComponent';
import { ElementProps, SystemProps } from '@material-ui/core/Box';

export interface Mark {
value: number;
Expand Down Expand Up @@ -158,6 +159,10 @@ export interface SliderTypeMap<P = {}, D extends React.ElementType = 'span'> {
* @default 1
*/
step?: number | null;
/**
* System props to apply to the component.
*/
system?: ElementProps & SystemProps;
/**
* The track presentation:
*
Expand Down
4 changes: 4 additions & 0 deletions packages/material-ui-lab/src/SliderUnstyled/SliderUnstyled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,10 @@ SliderUnstyled.propTypes = {
* @default 1
*/
step: PropTypes.number,
/**
* System props to apply to the component.
*/
system: PropTypes.object,
/**
* The track presentation:
*
Expand Down
7 changes: 4 additions & 3 deletions packages/material-ui-system/src/breakpoints.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ export function handleBreakpoints(props, propValue, styleFromPropValue) {
}

function breakpoints(styleFunction) {
const newStyleFunction = (props) => {
const base = styleFunction(props);
const themeBreakpoints = props.theme.breakpoints || defaultBreakpoints;
const newStyleFunction = (componentProps) => {
const base = styleFunction(componentProps);
const themeBreakpoints = componentProps.theme.breakpoints || defaultBreakpoints;
const props = componentProps.system || componentProps;

const extended = themeBreakpoints.keys.reduce((acc, key) => {
if (props[key]) {
Expand Down
5 changes: 3 additions & 2 deletions packages/material-ui-system/src/spacing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,10 @@ function getStyleFromPropValue(cssProperties, transformer) {
}, {});
}

function spacing(props) {
const theme = props.theme;
function spacing(componentProps) {
const theme = componentProps.theme;
const transformer = createUnarySpacing(theme);
const props = componentProps.system || componentProps;

return Object.keys(props)
.map((prop) => {
Expand Down
5 changes: 3 additions & 2 deletions packages/material-ui-system/src/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@ function style(options) {
const { prop, cssProperty = options.prop, themeKey, transform } = options;

const fn = (props) => {
if (props[prop] == null) {
const propValue = props[prop] || props.system?.[prop];

if (propValue == null) {
return null;
}

const propValue = props[prop];
const theme = props.theme;
const themeMapping = getPath(theme, themeKey) || {};
const styleFromPropValue = (propValueFinal) => {
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui/src/Box/Box.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ type BoxStyleFunction = ComposedStyleFunction<
]
>;

type SystemProps = PropsFor<BoxStyleFunction>;
type ElementProps = Omit<React.HTMLAttributes<HTMLElement>, keyof SystemProps>;
export type SystemProps = PropsFor<BoxStyleFunction>;
export type ElementProps = Omit<React.HTMLAttributes<HTMLElement>, keyof SystemProps>;

export interface BoxProps extends ElementProps, SystemProps {
// styled API
Expand Down
35 changes: 34 additions & 1 deletion packages/material-ui/src/styles/muiStyled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
import styled from '@material-ui/styled-engine';
import { propsToClassKey } from '@material-ui/styles';
import {
borders,
compose,
display,
flexbox,
grid,
palette,
positions,
shadows,
sizing,
spacing,
typography,
css,
} from '@material-ui/system';
import defaultTheme from './defaultTheme';

const getStyleOverrides = (name, theme) => {
Expand Down Expand Up @@ -54,7 +68,22 @@ const variantsResolver = (props, styles, theme, name) => {
return variantsStyles;
};

const shouldForwardProp = (prop) => prop !== 'styleProps' && prop !== 'theme';
const shouldForwardProp = (prop) => prop !== 'styleProps' && prop !== 'theme' && prop !== 'system';

export const systemStyleFunction = css(
compose(
borders,
display,
flexbox,
grid,
positions,
palette,
shadows,
sizing,
spacing,
typography,
),
);

const muiStyled = (tag, options, muiOptions) => {
const name = muiOptions.muiName;
Expand All @@ -72,6 +101,10 @@ const muiStyled = (tag, options, muiOptions) => {
return variantsResolver(props, getVariantStyles(name, theme), theme, name);
});

if (muiOptions && muiOptions.useSystemProps) {
styles.push(systemStyleFunction);
}

return defaultStyledResolver(...styles);
};
return muiStyledResolver;
Expand Down