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

[Divider] Add text in divider #22285

Merged
merged 17 commits into from Aug 26, 2020
Merged

[Divider] Add text in divider #22285

merged 17 commits into from Aug 26, 2020

Conversation

sheerryy
Copy link
Contributor

@sheerryy sheerryy commented Aug 19, 2020 •
edited

This PR adds a feature to add text in Divider

Closes #21820

@sheerryy
Copy link
Contributor Author

@oliviertassinari This PR Closes issue # 21820. If you think this is a big PR, then Let me know I will try to divide it into small PRs.

@github-actions github-actions bot mentioned this pull request Aug 19, 2020
Closed
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 19, 2020
mnajdova
mnajdova reviewed Aug 20, 2020
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I really like the approach of just using children instead of a property on the Divider 👍

oliviertassinari
oliviertassinari reviewed Aug 20, 2020
View reviewed changes
docs/src/pages/components/dividers/TextDividers.tsx Outdated
}),
);

export default function TextDividers() {
Copy link
Member

Choose a reason for hiding this comment

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

Great start with this new demo. However, I think that we should aim to further simplify it. What about:

  1. We break the vertical and horizontal cases in two different demos
  2. We remove the header with "Toothbrush"
  3. We use the same lorem ipsum text between all the cases, stored inside a string variable. This should allow us to get an inline preview that displays the source automatically behind the demo if there are less than 15 LOCs in the render () part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have applied the changes. Kindly check this commit.

docs/src/pages/components/dividers/dividers.md Outdated Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.js Outdated 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
packages/material-ui/src/Divider/Divider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.js Outdated Show resolved Hide resolved
packages/material-ui/src/Divider/Divider.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the component: divider This is the name of the generic UI component, not the React module! label Aug 25, 2020
@oliviertassinari oliviertassinari changed the title WIP: [Divider] Add text in Divider [Divider] Add text in Divider Aug 25, 2020
@oliviertassinari oliviertassinari changed the title [Divider] Add text in Divider [Divider] Add text in divider Aug 25, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 25, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 25, 2020 •
edited

@material-ui/core: parsed: +0.29% , gzip: +0.26%

Details of bundle changes

Generated by 🚫 dangerJS against 41dc6a0

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 25, 2020
@oliviertassinari oliviertassinari force-pushed the 21820-text-inside-divider branch 2 times, most recently from 994d04e to 9677918 Compare August 25, 2020 15:11
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 25, 2020
@oliviertassinari oliviertassinari added the new feature New feature or request label Aug 25, 2020
@sheerryy
Copy link
Contributor Author

sheerryy commented Aug 25, 2020 via email

@mnajdova
Copy link
Member

@shehryarshoukat96 left suggestions that in my opinion would best match the usage of the classes.

sheerryy and others added 3 commits August 26, 2020 02:15
@sheerryy @mnajdova
Update packages/material-ui/src/Divider/Divider.d.ts
b01993f 
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
@sheerryy @mnajdova
Update packages/material-ui/src/Divider/Divider.d.ts
0b8e8e9 
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
@sheerryy
run yarn api:docs
3521344
@mnajdova mnajdova merged commit ce073da into mui:next Aug 26, 2020
@mnajdova
Copy link
Member

Thanks again for the contribution @shehryarshoukat96 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@mnajdova mnajdova mnajdova approved these changes

Assignees
No one assigned
Labels
component: divider This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Divider] Support displaying text inside
4 participants
@sheerryy @mui-pr-bot @oliviertassinari @mnajdova