Implement proper typography solution with vertical rhythm

Okey here is the first review of code. Still didn't install this to check how it actually look, for now this is just a patch code review.

Beside specific points couple of general questions:

• Why typey is selected on the end - is it better from other frameworks or are they all more or less the same? Any advantages/disadvantages?
• What ever framework we choose we should add some resource links in documentation and maybe also code variables, sass files in that way that its easy to completely remove or change this framework with some other framework in child theme.

1. +++ b/themes/bs_bootstrap/css/layout/layout.css
   @@ -42,6 +45,7 @@
   +/*! typey | GPLv2 License | https://github.com/jptaranto/typey */
   
   +++ b/themes/bs_bootstrap/css/theme/print.css
   @@ -1,3 +1,4 @@
   +/*! typey | GPLv2 License | https://github.com/jptaranto/typey */
   
   +++ b/themes/bs_bootstrap/css/theme/theme.css
   @@ -1,67 +1,163 @@
   +/*! typey | GPLv2 License | https://github.com/jptaranto/typey */
   +/*! typey | GPLv2 License | https://github.com/jptaranto/typey */
   

   I don't like seeing this comments in our CSS compiled files. Its fine that they are in sources and sass files but not in compiled files. Can we remove them somehow?

2. +++ b/themes/bs_bootstrap/css/theme/theme.css
   @@ -1,67 +1,163 @@
   +  background-color: color(text-bg);
   +  color: color(text);
   ...
   +  background-color: color(mark-bg);
   +  color: color(text);
   

   This looks wrong, what are this text-bg, text and mark-bg strings in CSS? Maybe sass is missing $ sign for this vars or something else?

3. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
   @@ -1,69 +1,225 @@
   +// Root
   

   All this rules in this files came from where, are they recommended start rules for typey or is this our custom rules?

4. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
   @@ -1,69 +1,225 @@
   +* {
   +  box-sizing: border-box;
   +}
   

   BS _reboot.scss is doing

   html {
     box-sizing: border-box;
   }
   
   *,
   *::before,
   *::after {
     box-sizing: inherit;
   }
   

   Are you sure we need this also?

5. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
   @@ -1,69 +1,225 @@
   +  -ms-text-size-adjust: 100%;
   +  -webkit-text-size-adjust: 100%;
   

   Is it possible to use unprefixed text-size-adjust and then let autoprefixer to generate prefixed rules?

6. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
   @@ -1,69 +1,225 @@
   +  // On short pages, we want any background gradients to fill the entire height
   +  // of the browser.
   +  min-height: 100%;
   

   Hmmm not sure about this assumption or at least that this should be in base theme.

   Also how is this related to typography?

7. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
   @@ -1,69 +1,225 @@
   +  background-color: color(text-bg);
   +  color: color(text);
   

   Yeah here are the missing $ stuff.

8. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
   @@ -1,69 +1,225 @@
   +  text-decoration: underline;
   +  text-decoration: underline dotted;
   

   Why not just one rule here?

9. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
   @@ -1,69 +1,225 @@
   +  bottom: -0.25em;
   

   Maybe use some vertical grid generator here or its fine that this is fixed?

10. +++ b/themes/bs_bootstrap/sass/theme/base/_elements.scss
    @@ -1,69 +1,225 @@
    +%h1 {
    

    This %h1 is for what exactly?

11. +++ b/themes/bs_bootstrap/sass/variables/_bs_bootstrap.scss
    @@ -5,3 +5,67 @@
    +        xxl:              32px,
    +        xl:               24px,
    +        l:                20px,
    +        m:                $base-font-size,
    ...
    +        bold:             bold,
    +        medium:           500,
    

    I think indentation should be two spaces here.

12. +++ b/themes/bs_bootstrap/sass/variables/_bs_bootstrap.scss
    @@ -5,3 +5,67 @@
    +        monospace: (
    +                font-family: $menlo,
    +        ),
    

    Two spaces indentation.

Please check latest patch because lots of stuff is removed which are not related to vertical grid support - check some comments bellow - i guess this is an error.

> 1. This comments are coming from imported typey library. I couldn't find how to fix this. Do you have any suggestion?

We can do this easily in gulp process. We don't want to remove all comments but just this boring repeating stuff - something like some plugin (maybe we already have it) that will search&replace and then kill this comments in compiled CSS.

> 9.
>
> Maybe use some vertical grid generator here or its fine that this is fixed?
> Any suggestion here.

No, then i would need to learn how typey works ;) I am just asking can we apply something from typey so this component follows vertical grid?

1. +++ b/README.md
   @@ -117,3 +117,5 @@ browsersSupport: [
   +- https://github.com/jptaranto/typey
   +  A complete framework for working with typography on the web - applied in BS Bootstrap child theme.
   

   If implementation in bs_bootstrap then the documentation should be also in bs_bootstrap/README.md.

   However it make sense to promote this in bs_base because vertical grid framework is not related to BS. Can we maybe move this to bs_base somehow and then reuse it in bs_bootstrap?

2. +++ b/themes/bs_bootstrap/bs_bootstrap.libraries.yml
   @@ -11,7 +11,6 @@ global-styling:
   -      css/components/pagination.css: {}
   
   +++ b/themes/starterkit_bootstrap/starterkit_bootstrap.libraries.yml
   @@ -11,7 +11,6 @@ global-styling:
   -      css/components/pagination.css: {}
   

   Why is this removed?

3. +++ b/themes/bs_bootstrap/bs_bootstrap.libraries.yml
   @@ -91,22 +90,3 @@ util:
   -
   -modal:
   -  css:
   -    component:
   -      css/components/modal.css: {}
   -  js:
   -    js/bootstrap/modal.js: {}
   -  dependencies:
   -    - core/jquery
   -    - bs_bootstrap/util
   -
   -carousel:
   -  css:
   -    component:
   -      css/components/carousel.css: {}
   -  js:
   -    js/bootstrap/carousel.js: {}
   -  dependencies:
   -    - bs_bootstrap/util
   

   Removed why?

4. +++ b/themes/bs_bootstrap/gulp-options.yml
   @@ -26,5 +26,3 @@ bootstrap:
   -    - 'js/dist/modal.js'
   -    - 'js/dist/carousel.js'
   

   Yeah pretty confused with this removes?

5. +++ b/themes/bs_bootstrap/sass/variables/_bs_bootstrap.scss
   @@ -5,3 +5,68 @@
   +// Typey variables
   +// Documentation on the [Typey homepage](https://github.com/jptaranto/typey).
   

   This should not be in _bs_bootstrap.scss but in separated sass variable file called _typey.scss for exmaple

6. +++ /dev/null
   @@ -1,91 +0,0 @@
   -{#
   -/**
   - * @file
   - * Theme override to display a pager.
   

   More strage removes?

> What is the best solution for this?

Yeah just decouple stuff in sass so you don't hard bundle typey with other stuff but you keep stuff separated. Then child themes can easily override or remove this stuff in sass compilation. Also keep typey sass vars separated - there is a comment in previous comment about that.

Nice article discussing vertical rhythm implementation https://www.lullabot.com/articles/designing-rhythm-and-proportion.

Just figure it out that latest Bootstrap is using new rfs library for responsive fonts https://github.com/twbs/rfs. We should switch using this.