Skip to content

Add new rounded sizes classes - #28011

Merged
mdo merged 12 commits into
twbs:v4-devfrom
jt143:add-new-rounded-classes
Jan 13, 2019
Merged

Add new rounded sizes classes#28011
mdo merged 12 commits into
twbs:v4-devfrom
jt143:add-new-rounded-classes

Conversation

@jt143

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

Copy link
Copy Markdown
Contributor

jt143 added 3 commits January 9, 2019 22:18
@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

@XhmikosR XhmikosR left a comment

Copy link
Copy Markdown
Member

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

XhmikosR commented Jan 9, 2019

Copy link
Copy Markdown
Member

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

jt143 commented Jan 9, 2019

Copy link
Copy Markdown
Contributor Author

updated!

@MartijnCuppens

MartijnCuppens commented Jan 9, 2019

Copy link
Copy Markdown
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

@mdo mdo left a comment

Copy link
Copy Markdown
Member

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

jt143 commented Jan 10, 2019

Copy link
Copy Markdown
Contributor Author

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

@MartijnCuppens MartijnCuppens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

mdo
mdo approved these changes Jan 13, 2019
View reviewed changes

@mdo mdo left a comment

Copy link
Copy Markdown
Member

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
@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

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jt143 @XhmikosR @MartijnCuppens @mdo