-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
I went through the sample CSS for the “Getting Started” pages, reordered rulesets properties to an alpha order, removed properties not needed (based on last 2 versions of major browsers and IE8+) and removed unnecessary property values (e.g., em units on line-height).
@AllThingsSmitty It looks like the license agreement has not been signed. You will need to Accept the Brackets CLA before we can merge this pull request. |
Done. I did this yesterday, too, so not sure why it doesn't show. |
@abose any concerns with this PR? Thanks. |
@AllThingsSmitty Didn't get enough time to review this yet. |
} | ||
|
||
h1, h2, h3, h4, h5, h6 { | ||
color: #222; | ||
font-weight: 700; | ||
line-height: 1.3em; | ||
font-weight: 600; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
font-weight: 700
is now font-weight: 600
. It matches the others, but might be worth it to double check if the zh-tw
font requires the heavier weight.
LGTM with one question about |
@petetnt good point, I hadn't thought of that. I've reverted it back to the prior font-weight for |
@AllThingsSmitty Release 1.7 will be out this week or the next, so retargeted this for release 1.8 due to inadequate testing window for large changes like this. |
@abose sounds good. Looking forward to it. |
LGTM. |
I went through the sample CSS for the “Getting Started” pages,
reordered rulesets properties to an alpha order, removed properties not
needed (based on last 2 versions of major browsers and IE8+) and
removed unnecessary property values (e.g., em units on line-height).