search

jhildenbiddle / vertical-rhythm-reset

A Sass/SCSS library for responsive vertical rhythm grids, modular scale typography, and CSS normalization.
https://jhildenbiddle.github.io/vertical-rhythm-reset/
MIT License
76 stars 6 forks source link

Reset is resetting a little too hard, maybe? #2

Closed Lamecarlate closed 5 years ago

Lamecarlate commented 6 years ago

Hello!

I wonder why in the reset we have this line:

*,
*:before,
*:after {
  /* some properties */
  font: inherit;
}

Why the entire font, and not just font-family and font-size ? They're the only properties that could affect vertical rhythm, aren't they? The current problem I have with this is that font-weight and font-style are forced to "normal", so I have to re-style i, and em and strong. Is it normal, is it intentional?

On another topic: vertical-align: bottom kind of break sup and sub, it would be nice to give them again their respective alignments of "super" and "sub" in the reset. Would you like me to create a PR for that?

jhildenbiddle commented 6 years ago

All fair points, @Lamecarlate. 😄

I've made a handful of changes in a separate reset_update branch. Here are the notes from the CHANGELOG in that branch:

A few items worth mentioning:

  1. With the current version (1.1.1), both <b> and <strong> elements should be styled with font-weight: bolder. You can verify this by viewing the demo on Codepen.io. Perhaps you have some other reset code in your app that is setting these elements to font-weigh: normal? Here are the styles added by _vertical-rhythm-reset.scss:

    https://github.com/jhildenbiddle/vertical-rhythm-reset/blob/1998494111135d738ffff71b51b67020be57236d/dist/_vertical-rhythm-reset.scss#L1399-L1403

  2. You are correct about the missing <i> and <em> styles though. Not sure how I missed those, but they've been added to the (soon to be released) update.

  3. Replacing the universal font: inherit reset rule with font-family: inherit; font-size: inherit; means elements will render with their default font-weight and font-style values. This isn't necessarily a bad thing, but it is different than the currently released version (1.1.1). There are two paths forward, and I'm wondering which you prefer:

    1. Replace the font: inherit reset rule and let elements render using their default font-style and font-weight values. This means elements like <h1>-<h6>, <th>, and <optgroup> will render using font-weight: bold via user agent styles. This is other CSS resets like normalize.css behave.

    2. Retain the font: inherit reset rule and specify font-style and font-weight properties for specific elements like <b>, <strong>, <i>, and <em>. This will keep the output similar to the currently released version, but ensure that the elements most expect to be bold or italic are styled appropriately.

    FWIW, I'm leaning towards the first option.

Remember, these changes are not yet available via npm/unpkg, so you'll need to manually download _vertical-rhythm-reset.scss to test out the changes. Once I get a thumbs up, I'll publish a new version with the changes (1.2.0).

Thanks for taking the time to provide your feedback. Much appreciated!

Lamecarlate commented 6 years ago

Oh my, I'm sorry I didn't answer… (well, I had network problems, then no computer for days)

I had no time to test the points you're mentionning, I'll do that soon.

jhildenbiddle commented 6 years ago

No worries, @Lamecarlate. Just let me know when you've had time to review along with any feedback you may have. As I said, once I get a thumbs up I'll push 1.2.0 to NPM.

jhildenbiddle commented 5 years ago

Changes have been released as 1.2.0.