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

[Divider] Add support for middle divider #13574

Merged
merged 6 commits into from
Nov 16, 2018

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Nov 11, 2018

Thought adding these would be a quick win :) Sadly changing to variant makes this a breaking change. The old props can be added in an a warning shown to change this.

Upgrade path

We are introducing a new variant to the divider component: middle. Following our API guideline, we can no longer use a boolean property, it needs to be an enum, hence the introduction of the variant property.

import Divider from '@material-ui/core/Divider';

-<Divider inset />
+<Divider variant="inset" />

@oliviertassinari
Copy link
Member

@joshwooding I believe it's a breaking change. We will have to way ~3 months before being able to merge. Maybe you can make it a non breaking change (backward compatible) with a warning?

@joshwooding
Copy link
Member Author

@oliviertassinari That should do it :)

pages/api/divider.md Outdated Show resolved Hide resolved
pages/api/divider.md Outdated Show resolved Hide resolved
docs/src/pages/demos/dividers/dividers.md Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.test.js Outdated Show resolved Hide resolved
docs/src/pages/demos/dividers/InsetDividers.js Outdated Show resolved Hide resolved
docs/src/pages/demos/dividers/InsetDividers.js Outdated Show resolved Hide resolved
classes: PropTypes.object.isRequired,
};

export default withStyles(styles)(MiddleDividers);
Copy link
Member

Choose a reason for hiding this comment

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

Could we get a demo that's closer to the recommended use of middle dividers?

image

It doesn't have to be that detailed, just following the correct use case.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree

packages/material-ui/src/Divider/Divider.js Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.js Outdated Show resolved Hide resolved
@mbrookes mbrookes added component: divider This is the name of the generic UI component, not the React module! and removed breaking change labels Nov 13, 2018
@mbrookes mbrookes removed this from the v4 milestone Nov 13, 2018
[Divider] Add more tests
[docs] Updated Divider documentation
[Divider] Added deprecation warning when using inset property
[Divier] Re-added tests for inset property and added warning test
@oliviertassinari oliviertassinari added the deprecation New deprecation message label Nov 15, 2018
@oliviertassinari oliviertassinari force-pushed the add-more-dividers branch 3 times, most recently from 64ef674 to 547be2c Compare November 15, 2018 22:39
@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Nov 15, 2018
@oliviertassinari oliviertassinari changed the title [Divider] Add other dividers from the v2 spec [Divider] Add support for middle divider Nov 15, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 15, 2018

I have made some changes. I have removed the subheader property. It's important to have a simple Divider component that people can override.

@joshwooding
Copy link
Member Author

So, I've been working on the demos and I've noticed some pain points:

  • Subheaders should align with the list padding and currently they are set to a hard coded value whilst the lists are dynamic making subheaders look weird on big screens compared to small screens.

Big Screen

image

Small Screen

image

Styling Subheader Dividers are a bit fiddly at the moment, due to a them being composed of two different components, maybe a composition pattern would work better:

<Divider> <Typography>Subheader</Typography> </Divider>

this would allow for more customisability and easier styling, but would differ from the patterns we already have. As an example of less than ideal styling currently to add a border at the bottom of a Subheader Divider you need to add the margin to the text. We also always keep the current pattern and change the underlying components

@joshwooding
Copy link
Member Author

If you want them these are the current demos I've written excluding the subheader ones:

image

image

image

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 15, 2018

Subheaders should align with the list padding and currently they are set to a hard coded value whilst the lists are dynamic making subheaders look weird on big screens compared to small screens.

You are right, but it's not directly related. We are already impacted by the problem, there is a listitem issue open to solve it. The solution is 👌, it's described in the related issue: #10044. As far as I remember, we can make the spacing screen size invariant.

@oliviertassinari
Copy link
Member

A new demo

capture d ecran 2018-11-16 a 18 58 50

@oliviertassinari oliviertassinari merged commit 39066cc into mui:master Nov 16, 2018
@oliviertassinari
Copy link
Member

@joshwooding Thank you

@joshwooding joshwooding deleted the add-more-dividers branch November 16, 2018 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: divider This is the name of the generic UI component, not the React module! deprecation New deprecation message design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants