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

Fix Bug 1491154, switch from Verdana to Arial #5023

Merged
merged 1 commit into from Oct 22, 2018

Conversation

Projects
None yet
4 participants
@schalkneethling
Collaborator

schalkneethling commented Oct 4, 2018

This switches to Arial from Verdana. As a result I had to also make some tweaks to the typography. It is not a 1:1 match, but then it is not intended to be. Let me know your thoughts. Thanks!

@@ -20,7 +20,7 @@ homepage masthead (search, feature callouts)
}
h1 {
@include set-font-size(36px);
@include set-font-size(42px);

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 10, 2018

Contributor

Shouldn’t this be using $h2-font-size or another such variable?

This comment has been minimized.

@schalkneethling

schalkneethling Oct 10, 2018

Collaborator

Well, yes and no ;) I am working on the side on a more modular scale for our typography with some meaningful class/variable names. It just feels strange to me to set the font-size for an h1 to $h2-font-size

@hobinjk

The media/gaia changes are unnecessary depending on when this lands in relation to #5031 but otherwise this looks good to me

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 15, 2018

@schalkneethling can you rebase? PR #5031 was merged first.

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1491154-switch-body-text-to-arial branch from fba17a2 to f088e94 Oct 16, 2018

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Oct 16, 2018

@jwhitlock rebased.

@@ -37,7 +37,7 @@
}
li li {
@include bidi(((padding-left, $grid-spacing, padding-right, 0),));
@include bidi(((padding-left, $grid-spacing, padding-right, 0)));

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 16, 2018

Contributor

Looks like there’s an error in calling this.

/*
Trouble shooting:
- Check that you have the right number of nested (((parentheses)))
- Check that there is a trailing comma:
@include bidi(((float, left, right),));
*/

Based on the above, this needs the trailing comma which you removed.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 16, 2018

Collaborator

Ooooh, bad rebase. Will fixup.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 16, 2018

Collaborator

Oh no, it was prettier that removed the ,

@schalkneethling schalkneethling force-pushed the schalkneethling:bug1491154-switch-body-text-to-arial branch from f088e94 to 518c281 Oct 16, 2018

@jwhitlock

This changed a few more things than I expected, such as losing the relative sizes of body text versus form elements, notices, etc. I spot-checked a few, and they appear OK.

@jwhitlock jwhitlock merged commit ed60513 into mozilla:master Oct 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No new, high severity issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment