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

Set font-sizes relative to font-size-base #24060

Merged
merged 5 commits into from Oct 19, 2017
Merged

Set font-sizes relative to font-size-base #24060

merged 5 commits into from Oct 19, 2017

Conversation

tillmon
Copy link
Contributor

@tillmon tillmon commented Sep 23, 2017

This PR closes #24056

@andresgalante
Copy link
Collaborator

😄 nice, I like this one!

What about $lead-font-size ? should we base it on $font-size-base too?

$h4-font-size: 1.5rem !default;
$h5-font-size: 1.25rem !default;
$h6-font-size: 1rem !default;
$headings-font-size-base: $font-size-base !default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't introduce a new base for headers

@andresgalante
Copy link
Collaborator

andresgalante commented Oct 11, 2017

@mdo @XhmikosR I took another look at this PR. We have several font-size references across _variables.scss, and since they are all set in rem (or %) it might be easier to just to change the font-size base definition from body here:
https://github.com/twbs/bootstrap/blob/v4-dev/scss/_reboot.scss#L60

to under html here:
https://github.com/twbs/bootstrap/blob/v4-dev/scss/_reboot.scss#L28

That way by changing the font-size-base variable it will change the root and everything will follow. No changes needed any other variable and no extra layer of abstraction.

This would be a breaking change, and more dramatic than just making fonts sizes follow $font-size-base

I am ok with either, if you think we should continue with it. I can send a PR PR or attach a commit to this one to clean it up depending on the direction we want to go.

@mdo mdo merged commit 0aa8cbe into twbs:v4-dev Oct 19, 2017
@mdo mdo mentioned this pull request Oct 19, 2017
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Oct 21, 2020
PR for joomla#31184

I took a look at this to create a PR the correct way for bootstrap scss

We should just have to change one variable
`$font-size-base:             	    0.875rem; //instead of 1rem`

But the rest of the font variables have not been defined correctly (probably based on bs4 beta1?) and they should be changed as well to be relative to this variable.

See the commit to bootstrap 3 years ago where this was done twbs/bootstrap#24060

There may be other font sizes in our scss that need to be updated to use this variable - I have not checked yet

I have no opinion on a base font size of 16px (before) and 14px (with this PR)

To test you will need to run `npm ci` or `node build.js --compile-css`
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Oct 22, 2020
PR for joomla#31184

I took a look at this to create a PR the correct way for bootstrap scss

We should just have to change one variable
`$font-size-base:             	    0.875rem; //instead of 1rem`

But the rest of the font variables have not been defined correctly (probably based on bs4 beta1?) and they should be changed as well to be relative to this variable.

See the commit to bootstrap 3 years ago where this was done twbs/bootstrap#24060

There may be other font sizes in our scss that need to be updated to use this variable - I have not checked yet

I have no opinion on a base font size of 16px (before) and 14px (with this PR)

To test you will need to run `npm ci` or `node build.js --compile-css`
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.

None yet

5 participants