Skip to content
This repository was archived by the owner on May 4, 2020. It is now read-only.

Conversation

@lusimeon
Copy link
Contributor

@lusimeon lusimeon commented Aug 8, 2019

Add a base font-size variable to calculate type sizes rather than use default size (16px)

@lusimeon lusimeon added the bug Something isn't working label Aug 8, 2019
@lusimeon lusimeon requested a review from titouanmathis August 8, 2019 09:45
Copy link
Contributor

@titouanmathis titouanmathis left a comment

Choose a reason for hiding this comment

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

Some documentation is missing and a condition is missing, seems good to me otherwise :)

@lusimeon
Copy link
Contributor Author

@titouanmathis If we merge this branch, spaces will be now evaluated based on $font-size-base instead of 16px because of $spaces-base: 8px / 16px * 1rem;.
1rem = $font-size-base instead of 1rem = navigator font size before.

Maybe we should change the $spaces-base to a single value like $spaces-base: 8px; and then in _spaces.scss:

@function space($space) {
  ...
  // Return the value times the base value if our space is a number, or else we
  // return the whole value, allowing us to set `auto` in our `$spaces` list.
  @if (type-of($value) == 'number') {
    @return $value * ($spaces-base / $font-size-base * 1rem);
  } @else {
    @return $value;
  }
}

Unfortunaly, it will create a strong dependency between _typography.scss ($font-size-base) and _spaces.scss ($spaces-base) files…

@titouanmathis
Copy link
Contributor

Unfortunaly, it will create a strong dependency between _typography.scss ($font-size-base) and _spaces.scss ($spaces-base) files…

@lusimeon I think it will be easier to maintain over time if we keep the typography and spaces configuration separate. But the specificity of this configuration should definitely be documented so that people are aware of it.

@titouanmathis
Copy link
Contributor

There is something troubling me in this PR. The ability to set the root font size on which all other sizes are based is definitely a good feature, but this SCSS toolkit CSS output only contains classes and has no side effect on HTML elements. So I think that setting the root font size directly on the html element might not be the best way to set it.

Maybe we should just add a new .type-root-size class that sould be added to the html element on each project.

@titouanmathis
Copy link
Contributor

I am changing the base of this PR from the develop branch to the release/3.0.0 branch in order to include this feature in the future v3.0.0 😉

@titouanmathis titouanmathis changed the base branch from develop to release/3.0.0 August 19, 2019 19:54
@titouanmathis titouanmathis force-pushed the feature/root-font-size branch 2 times, most recently from 1a65d15 to ffa5b84 Compare August 19, 2019 20:01
@titouanmathis titouanmathis force-pushed the feature/root-font-size branch from ffa5b84 to b4cf846 Compare August 19, 2019 20:08
@titouanmathis
Copy link
Contributor

I merge this PR to send it in the first beta release of the v3.0.0 😉

@titouanmathis titouanmathis merged commit c6cb062 into release/3.0.0 Aug 19, 2019
@titouanmathis titouanmathis deleted the feature/root-font-size branch August 19, 2019 20:14
@titouanmathis titouanmathis mentioned this pull request Aug 20, 2019
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants