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

Update font-sizes value to use the REM unit and line-heights to be unitless #126

Open
wants to merge 30 commits into
base: 2.0-preview
Choose a base branch
from

Conversation

lozandier
Copy link

@lozandier lozandier commented Feb 19, 2017

PR Summary

Update font-sizes value to use the REM unit and line-heights to be unitless to be more consistent of the relative unit values used already to establish consistent typography while simultaneously being more accommodating of RWD & a11y-friendly uses of the the declarations.

PR Goals

  • Be more friendlier to override in common RWD & a11y-friendly ways
  • Be more consistent with the relative unit values already being used with widely supported units (REM is effectively supported by all browsers within the browser support matrix of Polymer 2 and then some)
  • Encourage developers from the very beginning to leverage RWD & a11y-friendly ways of setting type values, in addition using units that's more "F.I.R.S.T" component-friendly to establish their typography.

Recommended reviewer

  • @notwaldorf (Last Official Polymer team member to submit a change to this component re:2.0)

Things I recommend reviewer(s) to think about

  • Is there value adding a comment alongside such values about their pixel value origins for the sake of dev conveniency?

unitless to be more consistent of the relative unit values used already
to establish consistent typography while simultaneously being more
accommodating of RWD & a11y-friendly uses of the the declarations.
@notwaldorf
Copy link
Contributor

Oof, so we took all the values from the MD spec, which doesn't use rems. Something like line-height: 1.071428571; looks really confusing to me, and so does font-size: 2.8125rem;. It's very hard to understand what the size would actually be :(

I'm gonna cc @frankiefu and @cdata and see if they have any opinions

@notwaldorf
Copy link
Contributor

One more thought about the non-readability: I'm a bit worried that if you're a new user and look at typography.html you won't know which class you might want, because the rems aren't exactly...specific. Also, not all elements support randomly changing the font size like that (i think if you change the font-size in the html style, everything gets resized), without maybe adjusting some other padding, and some elements might, in some cases, look borked.

@lozandier
Copy link
Author

Yeah, it seems to me there's significant value commenting alongside the values the "px" equivalent that also alleviates MD spec is still being adhered to.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@lozandier lozandier force-pushed the 2.0-preview branch 2 times, most recently from 0d3c28e to 18fa995 Compare April 16, 2017 04:20
@lozandier
Copy link
Author

lozandier commented Apr 16, 2017

@notwaldorf @bicknellr @cdata @frankiefu @tjsavage Branch updated again for not out-of-date reviewing, but it seems to for some reason the CLA status changes after rebasing using the newest version of 2-0.preview branch; even after making sure all my commits were consistently me without two different emails or anything; help?

Gonna try to do an interactive rebase and squash the newest commit for the sake of simplicity (and log history)… Actually, no that doesn't seem possible here. Seems the only other option would be to get the commit message and cherry pick into a new fork of paper-styles, but that seems "extra"

It seems to be a GoogleBot CLA bug…

@lozandier
Copy link
Author

lozandier commented Apr 16, 2017

@frankiefu @cdata @notwaldorf @tjsavage In addition to the accessibility & RWD reasons I originally pointed out, I forgot to point out using relative sizing instead of pixels within typography.html seems to align better with Polymer's mission statement to 'use the platform': Browser user agent stylesheets by default use relative font-sizing (% or em), I'm not sure yet the current reasons–even with the knowledge Material Design uses device pixels (DP) as the baseline unit to explain the visual language for the sake of convenience for general readers of the spec–justify deviating from the Web platform's out-of-the-box handling of typography that avoids pixels, as far as I know (AFAIK).

Since Polymer helps Web professionals develop Components easier, faster, more reliably, & overall more elegantly, what comes with that is enlightening such professionals–helping them avoid practices that can make their components problematic to reuse that's not immediately obvious.

Accordingly, forcing type within their components to be pixels instead of being relative to the base font-size by default seems to be a practice to avoid based on how native elements handle type by default and the established best practices the W3C (and the general Web community) has evangelized for years (AFAIK).

CSS W3C working group members such as Fantasai have shared many publications recommending the use of relative units over absolute values such as pixels such as this one ("Absolute units (like cm or px) are usually the wrong answer"), as well as W3C's formal tips & tricks document for Web creatives of all levels recommends the use of em over px – even when they generally understand the merit of sometimes using pixels (often use cases associated with print output using Web technology).

Furthermore, FWIW, the typography sizing of popular CSS frameworks such as Bootstrap & Foundation also use rem & em instead of pixels.

With newcomers in mind, it seems particularly helpful developers understand the value of using such relative units such as rem and em to establish type within their components instead of absolute values without a deliberate reason to when it's often too easy to use pixels in contexts ultimately not ideal for other developers who develop apps with their components over time–as well as actual end users of the apps using their components.

All that said, there's definitely value doing things like documenting the actual pixel values along side the relative units that's particularly useful for newcomers. I was thinking of merely doing things such as font-size: 2em; /* 32px */

Anyone have any particular thoughts on this since the original comments on the PR?

@JosefJezek
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants