Skip to content

[Divider] Fix height for vertical divider in a flexbox #19614

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

Merged
merged 8 commits into from
Feb 9, 2020
Merged

[Divider] Fix height for vertical divider in a flexbox #19614

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d03946
Init
captain-yossarian Feb 8, 2020
3b87743
Order
captain-yossarian Feb 8, 2020
974a37d
Remove empty space
captain-yossarian Feb 8, 2020
d91bbab
Remove redundant code
captain-yossarian Feb 8, 2020
9c0f867
update docs
captain-yossarian Feb 8, 2020
80fabbf
Adjust to convention
captain-yossarian Feb 8, 2020
6e1a679
Add point
captain-yossarian Feb 8, 2020
d4d91fa
polish
oliviertassinari Feb 9, 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
2 changes: 1 addition & 1 deletion docs/pages/api/divider.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">absolute</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | Absolutely position the element. |
| <span class="prop-name">classes</span> | <span class="prop-type">object</span> | | Override or extend the styles applied to the component. See [CSS API](#css) below for more details. |
| <span class="prop-name">component</span> | <span class="prop-type">elementType</span> | <span class="prop-default">'hr'</span> | The component used for the root node. Either a string to use a DOM element or a component. |
| <span class="prop-name">flexItem</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the divider will apply different strategy for height. |
| <span class="prop-name">flexItem</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the divider will apply adapt to a parent flex container. |
| <span class="prop-name">light</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the divider will have a lighter color. |
| <span class="prop-name">orientation</span> | <span class="prop-type">'horizontal'<br>&#124;&nbsp;'vertical'</span> | <span class="prop-default">'horizontal'</span> | The divider orientation. |
| <span class="prop-name">variant</span> | <span class="prop-type">'fullWidth'<br>&#124;&nbsp;'inset'<br>&#124;&nbsp;'middle'</span> | <span class="prop-default">'fullWidth'</span> | The variant to use. |
Expand Down
22 changes: 12 additions & 10 deletions docs/src/pages/components/dividers/VerticalDividers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const useStyles = makeStyles(theme => ({
backgroundColor: theme.palette.background.paper,
color: theme.palette.text.secondary,
'& svg': {
margin: theme.spacing(2),
margin: theme.spacing(1.5),
},
'& hr': {
margin: theme.spacing(0, 0.5),
Expand All @@ -29,14 +29,16 @@ export default function VerticalDividers() {
const classes = useStyles();

return (
<Grid container alignItems="center" className={classes.root}>
<FormatAlignLeftIcon />
<FormatAlignCenterIcon />
<FormatAlignRightIcon />
<Divider orientation="vertical" />
<FormatBoldIcon />
<FormatItalicIcon />
<FormatUnderlinedIcon />
</Grid>
<div>
<Grid container alignItems="center" className={classes.root}>
<FormatAlignLeftIcon />
<FormatAlignCenterIcon />
<FormatAlignRightIcon />
<Divider orientation="vertical" flexItem />
<FormatBoldIcon />
<FormatItalicIcon />
<FormatUnderlinedIcon />
</Grid>
</div>
);
}
22 changes: 12 additions & 10 deletions docs/src/pages/components/dividers/VerticalDividers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const useStyles = makeStyles((theme: Theme) =>
backgroundColor: theme.palette.background.paper,
color: theme.palette.text.secondary,
'& svg': {
margin: theme.spacing(2),
margin: theme.spacing(1.5),
},
'& hr': {
margin: theme.spacing(0, 0.5),
Expand All @@ -31,14 +31,16 @@ export default function VerticalDividers() {
const classes = useStyles();

return (
<Grid container alignItems="center" className={classes.root}>
<FormatAlignLeftIcon />
<FormatAlignCenterIcon />
<FormatAlignRightIcon />
<Divider orientation="vertical" />
<FormatBoldIcon />
<FormatItalicIcon />
<FormatUnderlinedIcon />
</Grid>
<div>
<Grid container alignItems="center" className={classes.root}>
<FormatAlignLeftIcon />
<FormatAlignCenterIcon />
<FormatAlignRightIcon />
<Divider orientation="vertical" flexItem />
<FormatBoldIcon />
<FormatItalicIcon />
<FormatUnderlinedIcon />
</Grid>
</div>
);
}
1 change: 1 addition & 0 deletions docs/src/pages/components/dividers/dividers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,6 @@ The examples below show two ways of achieving this.
## Vertical Dividers

You can also render a divider vertically using the `orientation` prop.
Note the usage of the `flexItem` prop to accommodate for the flex container.
Copy link
Member

@mbrookes mbrookes Feb 9, 2020

Choose a reason for hiding this comment

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

Suggested change
Note the usage of the `flexItem` prop to accommodate for the flex container.
Note the use of the `flexItem` prop to accommodate for the flex container.

Copy link
Member

@oliviertassinari oliviertassinari Feb 9, 2020

Choose a reason for hiding this comment

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

Thanks, I take care of it in a follow-up.


{{"demo": "pages/components/dividers/VerticalDividers.js", "bg": true}}
2 changes: 1 addition & 1 deletion packages/material-ui/src/Divider/Divider.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { OverridableComponent, OverrideProps } from '../OverridableComponent';
export interface DividerTypeMap<P = {}, D extends React.ElementType = 'hr'> {
props: P & {
absolute?: boolean;
light?: boolean;
flexItem?: boolean;
light?: boolean;
orientation?: 'horizontal' | 'vertical';
variant?: 'fullWidth' | 'inset' | 'middle';
};
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Divider/Divider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ Divider.propTypes = {
*/
component: PropTypes.elementType,
/**
* If `true`, the divider will apply different strategy for height.
* If `true`, the divider will apply adapt to a parent flex container.
Copy link
Member

@mbrookes mbrookes Feb 9, 2020

Choose a reason for hiding this comment

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

Suggested change
* If `true`, the divider will apply adapt to a parent flex container.
If `true`, the divider will adapt to a parent flex container.

?

(Fixes the grammar, but it's still not clear what this is actually trying to say.)

Copy link
Member

@oliviertassinari oliviertassinari Feb 9, 2020

Choose a reason for hiding this comment

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

I'm not sure how we could better explain the role of this prop.

Copy link
Member

@mbrookes mbrookes Feb 10, 2020

Choose a reason for hiding this comment

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

We could spell it out 🤷‍♂:

Suggested change
* If `true`, the divider will apply adapt to a parent flex container.
If `true`, a vertical divider will have the correct height when used in flex container. (By default, a vertical divider will have a calculated height of `0px` in Chrome if it is the child of a flex container.)

*/
flexItem: PropTypes.bool,
/**
Expand Down