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

CSS coding standards for font-weight #11715

Merged
merged 2 commits into from Nov 13, 2018

Conversation

Projects
None yet
6 participants
@Rahmon
Contributor

Rahmon commented Nov 10, 2018

Description

I've updated font-weight for numeric values (normal -> 400 and bold -> 600)

Closes: #11687

How has this been tested?

This has been tested with "npm test" and manually on Chrome and Firefox

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ajitbohra ajitbohra requested a review from jasmussen Nov 11, 2018

@Soean

Soean approved these changes Nov 12, 2018

@jasmussen

According to https://css-tricks.com/almanac/properties/f/font-weight/, "bold" maps to 700, not 600. Also, let's make sure we test this visually before we merge — I'm, 73% sure that there are specific instances where 600 was used instead of bold, because the semi bold version was desired.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 12, 2018

Thanks so very much for this PR! Really appreciate it. I left a comment that we should make sure to visually test that this doesn't change anything, as we're past the UI freeze. Bold is supposedly not the same as 600, so let's just be absolutely sure.

If you are busy and need help wrapping up those changes, let me know and I can volunteer to fix this up.

@Rahmon

This comment has been minimized.

Contributor

Rahmon commented Nov 12, 2018

@jasmussen

I've used 600 because in the issue (#11687) @afercia said 600 to bold. So do I need update to 700?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 12, 2018

Good question. His reasoning is that core uses 600 for bold, which certainly makes sense from a consistency point of view. Let me take this for a quick visual testing spin and revisit.

@Soean

This comment has been minimized.

Member

Soean commented Nov 12, 2018

Maybe its a typo, the link to the Coding standards says bold -> 700: https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#values

@ntwb

This comment has been minimized.

Member

ntwb commented Nov 12, 2018

Maybe its a typo, the link to the Coding standards says bold -> 700: make.wordpress.org/core/handbook/best-practices/coding-standards/css/#values

Not a typo, as mentioned above:

According to css-tricks.com/almanac/properties/f/font-weight, "bold" maps to 700, not 600.

@Rahmon

This comment has been minimized.

Contributor

Rahmon commented Nov 12, 2018

Maybe its a typo, the link to the Coding standards says bold -> 700: https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#values

I saw this link and I'd confused... 😕

@afercia

This comment has been minimized.

Contributor

afercia commented Nov 12, 2018

Background: core used "Open Sans" where 600 mapped to the Open Sans "semi bold" weight. Most of the CSS in core still uses "600". Actually, it depends on the font. If the font doesn't provide a "600" font weight, browsers will fall back to bold anyways.

See also #5848

More importantly, see https://core.trac.wordpress.org/changeset/37740

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 12, 2018

Thanks for the history. I just took this this for a visual spin, and my aging eyes see no visual regressions.

As such, I'm ready to approve this PR. But per comments by @ntwb and @Soean let me know what you think is the best approach here. 600 and 700 are both fine with me — just need to know we're doing the right thing, since it's about code quality.

@afercia

This comment has been minimized.

Contributor

afercia commented Nov 12, 2018

To my knowledge 600 is the standard in core. The handbook should be updated. For a final word, you may want to ask @helen

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 12, 2018

No need for any final words, 600 sounds right to me. Just want to verify with @ntwb per the above. Would also be good to track somewhere to update the handbook, just so it doesn't fall off the radar.

@jasmussen jasmussen self-requested a review Nov 12, 2018

@jasmussen

In effort to keep things moving, approving this per discussion points.

If this was in error, and it should be 700 instead of 600, feel free to ping me and I'll submit a PR to change that.

@jasmussen jasmussen added this to the 4.4 milestone Nov 12, 2018

@ntwb

This comment has been minimized.

Member

ntwb commented Nov 12, 2018

Thanks @afercia, I think the key part of that commit you cited is this:

"With the move to system fonts, we need to be specific rather than relying on the lack of a 700 weight. Not all system fonts include a 600 weight; in those instances, they will use the bold/700 weight."

So I guess we should perform an audit of the system font stack WordPress uses to determine which system fonts include font weight 700, or only 600.

Per the above, happy to see this merged as is, we should follow this up on Trac IMHO

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 13, 2018

If this was in error, and it should be 700 instead of 600, feel free to ping me and I'll submit a PR to change that.

Why not use variables instead of those hardcoded values?

Per the above, happy to see this merged as is, we should follow this up on Trac IMHO

Yes, but let's consider my previous comment.

@gziolo gziolo merged commit f8c4234 into WordPress:master Nov 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Rahmon Rahmon deleted the Rahmon:fix/11687 branch Nov 13, 2018

@ntwb

This comment has been minimized.

Member

ntwb commented Nov 13, 2018

Why not use variables instead of those hardcoded values?

These shouldn't really change value, ever

Plus, I think this would just be weird, not for any legitimate reason though /shrug

-font-weight: bold;
+font-weight: $bold;

Actually, the legit reason would be I'd have to update the stylelint font-weight-notation rule to allow variables as valid values instead of the current checks for if the value is numeric or not 😉

https://github.com/stylelint/stylelint/blob/master/lib/rules/font-weight-notation/index.js#L64

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

CSS coding standards for font-weight (WordPress#11715)
* Update font-weight from 'normal' to '400'

* Update font-weight from 'bold' to '600'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment