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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new `.rounded-pill` utility #27339

Merged
merged 37 commits into from Nov 5, 2018

Conversation

Projects
5 participants
@sts-ryan-holton
Contributor

sts-ryan-holton commented Sep 29, 2018

This PR adds a new .rounded-pill class as per MDO's comment on #26442 and also adds a new .btn-rounded class to the buttons component (https://getbootstrap.com/docs/4.1/components/buttons/)

Essentially, I've made a reusable mixin which is then applied to both the .rounded-pill and btn-rounded classes. I felt it wouldn't make sense to simply apply a .rounded-pill class directly to a .btn as MDO has mentioned in many conferences about creating modifier classes, it only makes logical sense to then create a .btn-rounded class to follow this structure.

This PR also updates the docs with these new classes 馃憤

Hoping we can add this into Bootstrap 4.2, and hope to see it as a useful feature as I received positive community upvotes with this 鉂わ笍

/CC @XhmikosR @mdo

Fixes #26442

This fixes #26442 by adding a new .rounded-pill utility class, and a 鈥
鈥ew .btn-rounded class fur buttons following MDO's suggestions. Also created a mixin that serves both of these classes.

@XhmikosR XhmikosR requested review from mdo and andresgalante Sep 29, 2018

Show resolved Hide resolved scss/utilities/_borders.scss Outdated
@sts-ryan-holton

This comment has been minimized.

Contributor

sts-ryan-holton commented Sep 29, 2018

/cc @XhmikosR Switched from 50px to 3.125rem. Tested on .btn, .btn-sm and .btn-lg

@XhmikosR

This comment has been minimized.

Member

XhmikosR commented Sep 29, 2018

Personally I don't know if such a big value is needed. From my quick testing 1.5rem was enough.

/CC @MartijnCuppens for thoughts :)

@sts-ryan-holton

This comment has been minimized.

Contributor

sts-ryan-holton commented Sep 29, 2018

/cc @XhmikosR You're right! I missed that, I retested the sizing, switched to 1.5rem now

Show resolved Hide resolved scss/utilities/_borders.scss Outdated

sts-ryan-holton and others added some commits Sep 29, 2018

@XhmikosR XhmikosR changed the title from Adds new .btn-rounded and .rounded-pill classes as per #26442 to Adds new .btn-rounded and .rounded-pill classes Sep 29, 2018

@XhmikosR XhmikosR changed the title from Adds new .btn-rounded and .rounded-pill classes to Add new .btn-rounded and .rounded-pill classes Sep 29, 2018

Show resolved Hide resolved scss/mixins/_border-radius.scss Outdated
Show resolved Hide resolved site/docs/4.1/utilities/borders.md Outdated
Show resolved Hide resolved scss/utilities/_borders.scss Outdated
@sts-ryan-holton

This comment has been minimized.

Contributor

sts-ryan-holton commented Sep 29, 2018

Revised edits:

  • Remove unnecessary rounded-pill mixin.
  • Update docs page.
  • Revise the .btn-rounded modifier class

/cc @MartijnCuppens

@sts-ryan-holton sts-ryan-holton changed the title from Add new .btn-rounded and .rounded-pill classes to Add new .btn-rounded class Sep 30, 2018

Show resolved Hide resolved site/docs/4.1/utilities/borders.md Outdated
@sts-ryan-holton

This comment has been minimized.

Contributor

sts-ryan-holton commented Sep 30, 2018

/cc @MartijnCuppens How do you suggest implementing the changes for the .rounded-pill as I noticed you used 50rem in your example.

Are you suggesting to keep the .btn-rounded class at 1.5rem and have a .rounded-pill at 50rem

@mdo

This comment has been minimized.

Member

mdo commented Sep 30, 2018

My comment in that PR wasn't about component-specific classes like these, but instead a new .rounded-* utility to that a single class can be used on any element/component.

@sts-ryan-holton

This comment has been minimized.

Contributor

sts-ryan-holton commented Sep 30, 2018

/cc @mdo This was my original thinking, but a lot of people are likely to want to see the ability to create a rounded button using a .btn-rounded class and I thought it would look a little weird creating an example on the buttons component page with a .rounded-pill class on there?

Unless we create a section on the docs that points to the new .rounded-pill class?

Also, we'd have to go from 1.5rem to something like 50rem as shown in @MartijnCuppens demo to ensure it works.

@MartijnCuppens

This comment has been minimized.

Member

MartijnCuppens commented Sep 30, 2018

@mdo, wouldn't that be a little inconsistent with the other modifying classes of .btn? And badges also have a modifying class (https://getbootstrap.com/docs/4.1/components/badge/#pill-badges).


@sts-ryan-holton,

Are you suggesting to keep the .btn-rounded class at 1.5rem and have a .rounded-pill at 50rem

Yes. The 50rem I used in the demo was just a random large number, but I think this might a good value? I would definitely make them variable, so they can be overridden easily.

sts-ryan-holton added some commits Oct 21, 2018

@sts-ryan-holton sts-ryan-holton requested a review from twbs/css-review as a code owner Nov 2, 2018

@MartijnCuppens

I'm going to change my mind on this one and follow mdo. It does make sense to only use .rounded-pill. The .btn-rounded class only uses one property and we should use utility classes for this.

If we would add the .btn-rounded class we should also add the .btn-uppercase, .btn-bold, etc. classes and if we'll need to do this for every component, we'll end up with a lot of selectors that'll do exactly the same as the utility classes do.

@mdo

This comment has been minimized.

Member

mdo commented Nov 3, 2018

Thanks @MartijnCuppens鈥攁ppreciate the extra eyes and feedback. Same to @sts-ryan-holton for constantly dealing with our feedback :D.

sts-ryan-holton and others added some commits Nov 3, 2018

Remove the .btn-rounded class from buttons component, remove from SCS鈥
鈥, remove the btn-rounded variable, retain the .rounded-pill utility class and variable as discussed.
@sts-ryan-holton

This comment has been minimized.

Contributor

sts-ryan-holton commented Nov 4, 2018

I've made the relevant changes:

  • Remove .btn-rounded class & variable.
  • Remove .btn-rounded class mentioned in docs.

sts-ryan-holton and others added some commits Nov 4, 2018

@mdo

mdo approved these changes Nov 5, 2018

@XhmikosR XhmikosR added this to Inbox in v4.2 via automation Nov 5, 2018

@XhmikosR XhmikosR changed the title from Add new .btn-rounded class and .rounded-pill to Add new `.rounded-pill` utility Nov 5, 2018

@mdo mdo referenced this pull request Nov 5, 2018

Open

v4.2.0 shiplist #26952

@XhmikosR XhmikosR merged commit 89eef04 into twbs:v4-dev Nov 5, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on add-rounded-pill-feature at 90.966%
Details
deploy/netlify Deploy preview ready!
Details

v4.2 automation moved this from Inbox to Shipped Nov 5, 2018

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