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.

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

Variable .btn and .form-control font sizes #26908

Merged
merged 6 commits into from
Sep 18, 2018

Conversation

MartijnCuppens
Copy link
Member

Closes #26907.

Copy link
Collaborator

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

Hi @MartijnCuppens, thanks a lot! I like the idea of having variables to change the font size of the btns. Having said that there are 2 things that I'd change on this PR:

  • As @isychev mention on Change btn font-sizes #26907 there should be a relation between the font size of the btn and the font size of the input, the same way we do paddings for example.

  • This PR changes the font-size of the sm and lg btns and it'd produce a regresion. I suggest creating variables but defining them as as $font-size-base. Therefore we keep the same aspect, we don't break authors implementations but we open an option to style them.

Does it make sense?

@MartijnCuppens
Copy link
Member Author

Thanks for the review @isychev and @andresgalante. It makes sense to keep the btn and input font sizes in sync, I'll look into this.

This PR changes the font-size of the sm and lg btns and it'd produce a regresion. I suggest creating variables but defining them as as $font-size-base. Therefore we keep the same aspect, we don't break authors implementations but we open an option to style them.

Right now .btn-sm and .btn-lg buttons use $font-size-sm and $font-size-lg as font-size, are you suggesting to change this to $font-size-base, Andres?

@andresgalante
Copy link
Collaborator

Yes, I am suggesting something like:

// define inputs and btns to be the same
$input-btn-font-size: $font-size-base;
$input-btn-font-size-sm: $input-btn-font-size;
$input-btn-font-size-lg: $input-btn-font-size;

// and have control over the btns too
$btn-font-size: $input-btn-font-size;
$btn-font-size-sm: $input-btn-font-size-sm;
$btn-font-size-lg: $input-btn-font-size-lg;

Makes sense?

@MartijnCuppens
Copy link
Member Author

Hmm, wouldn't that be a breaking change? All the font-sizes of for the .form-control elements with sizing classes would be the same if we did that?
https://getbootstrap.com/docs/4.1/components/forms/#sizing

Wouldn't this make more sense?

// define inputs and btns to be the same
$input-btn-font-size: $font-size-base;
$input-btn-font-size-sm: $font-size-sm;
$input-btn-font-size-lg: $font-size-lg;

// and have control over the btns too
$btn-font-size: $input-btn-font-size;
$btn-font-size-sm: $input-btn-font-size-sm;
$btn-font-size-lg: $input-btn-font-size-lg;

@MartijnCuppens
Copy link
Member Author

I've updated my PR to keep the .btn and .form-control font sizes in sync to avoid inconsistencies between them. @andresgalante, can you review the changes I made?

I've also updated $custom-select-font-size-sm and $custom-select-font-size-lg. This will cause conflicts if we merge this together with #26585. Shall I merge the changes from that PR in this PR?

@MartijnCuppens MartijnCuppens changed the title Variable btn font sizes Variable .btn and .form-control font sizes Jul 24, 2018
@isychev
Copy link

isychev commented Jul 25, 2018

I think that this is a good idea. The main thing that all worked by default, If someone wants to change the size of the buttons, he must understand the risks. It may be worth adding a comment "Caution. Resizing only buttons can affect layout"

Copy link
Collaborator

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

👍 Thanks a lot @MartijnCuppens !

@mdo I'll add it to 4.2 since it's a new feature.

@mdo mdo mentioned this pull request Sep 18, 2018
@XhmikosR XhmikosR merged commit e604958 into twbs:v4-dev Sep 18, 2018
@MartijnCuppens MartijnCuppens deleted the variable-btn-font-sizes branch September 19, 2018 20:05
@XhmikosR XhmikosR mentioned this pull request Nov 16, 2018
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.

6 participants