Skip to content

Utility class border radius#30048

Closed
JensLuyten wants to merge 4 commits intotwbs:mainfrom
JensLuyten:utility-class-border-radius
Closed

Utility class border radius#30048
JensLuyten wants to merge 4 commits intotwbs:mainfrom
JensLuyten:utility-class-border-radius

Conversation

@JensLuyten
Copy link
Contributor

@JensLuyten JensLuyten commented Jan 17, 2020

Closes #29989

Updated the docs pages by searching for the .rounded classes and replacing them by the new .border-radius classes.

See
https://deploy-preview-30048--twbs-bootstrap.netlify.com/docs/4.3/utilities/borders/#border-radius
https://deploy-preview-30048--twbs-bootstrap.netlify.com/docs/4.3/utilities/borders/#sizes

MartijnCuppens
MartijnCuppens previously approved these changes Jan 17, 2020
@JensLuyten JensLuyten force-pushed the utility-class-border-radius branch from 26125ec to 8bee335 Compare January 17, 2020 13:28
@XhmikosR XhmikosR requested a review from mdo January 17, 2020 13:44
@JensLuyten
Copy link
Contributor Author

I have shortened the new .border-radius class to .br and updated the docs with the new information.

@tpaksu
Copy link

tpaksu commented Jan 21, 2020

Then "border radius bottom right" will be br-br, right? :)

@JensLuyten
Copy link
Contributor Author

Then "border radius bottom right" will be br-br, right? :)

The border radius class will apply to all 4 corners, so there won't be a br-br-2 class to just set the bottom right border radius with the current implementation.

@tpaksu
Copy link

tpaksu commented Jan 22, 2020

Then "border radius bottom right" will be br-br, right? :)

The border radius class will apply to all 4 corners, so there won't be a br-br-2 class to just set the bottom right border radius with the current implementation.

Yes, I guess I saw the implementation in tailwindcss. That came to my mind and wanted to ask. BTW the clarification with .border-radius is gone with .br. IMO, of course. If you want to add a line break class later for example .br { margin-bottom: 1em } this would clash. But there's also .mb-3, right. Well you know it better. Just my thoughts.

@MartijnCuppens
Copy link
Member

Not sure about the br-top syntax, in general we concat the properties (eg. pt for padding-top). This would mean we should use brt instead.

Not sure about that syntax either though. @mdo, what do you think?

@ysds
Copy link
Contributor

ysds commented Feb 10, 2020

This PR does not include rename the border class name, but as @mdo commented above, the new name for border must also be considered. (This does not mean to include it in this PR.)
 
We should avoid conflicting br (border-radius and border-right). The best solution here is to match Zen Cording.

e.g.

  • bd-1 : border: 1px solid $border-color;
  • bdt-1: border-top: 1px solid $border-color;
  • bdr-1: border-right: 1px solid $border-color;
  • bdrs-1: border-radius: $border-radius-sm:

This also compatible with the spacing classes.

@ysds
Copy link
Contributor

ysds commented Feb 13, 2020

@JensLuyten Could you change the class names to bdrs?

@mdo
Copy link
Member

mdo commented Feb 14, 2020

@ysds At that point, we might as well use radius instead of abbreviating :). I don't mind that fwiw.

@mdo
Copy link
Member

mdo commented Mar 4, 2020

@JensLuyten Could you please take a pass at updating this to reflect the latest comments? We're backpedaling on the abbreviations and are opting for radius as the clearest option.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Confusing classes rounded-sm and rounded-lg

6 participants