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

Add new rounded sizes classes #28011

Merged
merged 12 commits into from Jan 13, 2019
Merged

Add new rounded sizes classes #28011

merged 12 commits into from Jan 13, 2019

Conversation

jt143
Copy link
Contributor

@jt143 jt143 commented Jan 9, 2019 •
edited by MartijnCuppens

@jt143
update border-radius mixins
3df656f 
Add $enable-rounded as a keyword argument to border-raidus mixins
@jt143
Update rounded classes
9db8dd9 
- use border-radius mixins to repleace !important
- use true for $enable-rounded for rounded classes
- Add `.rounded-sm` and `.rounded-sm`  twbs#27934
@jt143
update borders docs
b0ea667
@jt143 jt143 requested a review from a team as a code owner January 9, 2019 11:40
@jt143 jt143 changed the title Update border-radius mixins, remove !important in rounded class, add rounded-sm and rounded-lg classes Update border-radius mixins, border-radius classes, add new rounded sizes classes Jan 9, 2019
XhmikosR
XhmikosR reviewed Jan 9, 2019
View reviewed changes
Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

Please do not touch any dist files.

@XhmikosR
Copy link
Member

XhmikosR commented Jan 9, 2019

Also, there's a good reason for using !important in utilities.

@jt143 jt143 changed the title Update border-radius mixins, border-radius classes, add new rounded sizes classes Add new rounded sizes classes Jan 9, 2019
@jt143
Copy link
Contributor Author

jt143 commented Jan 9, 2019

updated!

@MartijnCuppens
Copy link
Member

Thanks for the PR, @jt143!

These classes have !important because they are utility classes. The classes just do one thing and shouldn't depend on other classes.

Therefor I would go for .rounded-sm classes instead of .rounded.rounded-sm classes (same for .rounded-lg). And also .rounded-top-sm classes instead of .rounded-top.rounded-sm (and same behaviour for other variants).

mdo
mdo requested changes Jan 9, 2019
View reviewed changes
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

We wouldn't chain these new classes to the side classes like this. Utilities should never ben extended, chained, or modified. For now, I'd suggest we do .rounded-sm and .rounded-lg only.

@jt143
Copy link
Contributor Author

jt143 commented Jan 10, 2019

Hi @mdo and @MartijnCuppens
Thank you for your reviews! I have updated as requested.

MartijnCuppens
MartijnCuppens approved these changes Jan 10, 2019
View reviewed changes
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@MartijnCuppens MartijnCuppens added this to Inbox in v4.3 via automation Jan 12, 2019
@XhmikosR XhmikosR moved this from Inbox to Needs review/changes in v4.3 Jan 13, 2019
mdo
mdo approved these changes Jan 13, 2019
View reviewed changes
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Looks good!

@mdo mdo merged commit 8f5abf0 into twbs:v4-dev Jan 13, 2019
v4.3 automation moved this from Needs review/changes to Shipped Jan 13, 2019
@mdo mdo mentioned this pull request Jan 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XhmikosR XhmikosR XhmikosR left review comments

@mdo mdo mdo approved these changes

@MartijnCuppens MartijnCuppens MartijnCuppens approved these changes

Assignees
No one assigned
Labels
css feature v4
Projects
No open projects
v4.3
  
Shipped
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jt143 @XhmikosR @MartijnCuppens @mdo