-
Notifications
You must be signed in to change notification settings - Fork 1
Replace Open Sans with Neue Haas Grotesk as primary typeface on Wordpress #186
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
Conversation
Set NHG as the typeface in our _typography.scss file. Removed all other references to Open Sans to avoid having to debug overrides down the road.
…verison of font for headings
…to have appropriate font weight
…ell as visual bugs
…n of NHG in favor of adobe typekit delivered
…ed single post title spacing
…typography to match other portions of the site. Fixed subtitle color for accessiblity
…nd avoid wrapping on SM breakpoint
@matt-bernhardt This should be ready for review! Most important areas to spot check would be:
They have the most changes. I noted more in the PR message! Early-mid next week would be ideal, but no urgency if you need more time, just let me know! |
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.
In broad terms, this looks okay to my eye - and this is a massive amount of work, so thank you for taking it on.
I've tried to label individual comments as questions or requested changes, to be clear about what I think the needed tweaks are. One is a syntax problem (missing bracket), one is a desire to be consistent in how we handle stylesheets (even externally-hosted ones), and one is a desire not to directly modify third-party libraries. I'm open to pushback if you feel strongly that the directions you've chosen are superior, but let's talk about it.
The questions are all just that- I'll accept the change as-is, but I'd like to hear your thoughts.
I should caveat that, for the specific CSS changes spread throughout, I'm trusting that the stakeholders have approved the way the pages look now - I don't want to second-guess either their stewardship of the content / branding, nor how you've made that work in specific rules / etc. The pages you've listed all seem fine to me, nothing is obviously broken. My focus has been more on the engineering and how the stylesheets are put in place, rather than worrying about whether specifying px or em is preferred, or any of the branding considerations.
font-size: .9em; | ||
} | ||
} | ||
|
||
@media only screen and (min-device-width : 375px) and (max-device-width : 667px) and (orientation : portrait) and (-webkit-min-device-pixel-ratio : 2) { |
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.
Requested change Removing this line feels like it needs to also take out the closing bracket, but I don't see it removed in this changeset. Looking at the file now, I think there's a closing backet on line 255 which is no longer accounted for?
Comment only: this is likely beyond the scope of this PR, but I also find the lack of indenting within media queries throughout this file to be confounding - making it far easier for things like this to slip in. I know you didn't cause this, and this PR is already big enough, but what do you think of a separate ticket to deal with formatting like this?
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.
I'll remove the bracket in this PR. (Also found a missing semicolon that I can add in).
This file being static CSS is also a little peculiar. I wonder if the ticket should also include moving this into some SCSS file and having the file cleaned up a bit? Then we could use nesting when doing any indenting. What do you think?
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.
I agree with refactoring this to leverage SCSS as part of that ticket, yeah. Good call!
web/app/themes/mitlib-parent/css/scss/partials/pages/_about.scss
Outdated
Show resolved
Hide resolved
… styling to tables style partial
@matt-bernhardt I replied to all comments and uploaded all the commits with my fixes! This should be ready to review again. Note that the About page had some refactoring (which didn't appear to break anything on my end) but that commit might warrant extra scrutiny. |
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.
It looks like the last round of commits has resolved most things, although I don't see a tweak to hours-mobile. That said, I'm also not sure how to confirm what the behavior is for that set of rules, because I'm unclear how to trigger the pixel-ratio rule that is meant to wrap lines 231-255. Do you have a process I can follow to see those rules kick in?
I'm marking this as approved, just to get us closer to done - if it merges now I think the only negative impact might be for specific devices in specific configurations, and I don't want to be too much of a stickler.
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.
Looks good! Thanks for catching the Samsung Galaxy issue, and for the new Jira tickets.
Developer
The typeface our Wordpress site was using (Open Sans) did not fit into the same style of typeface as our brand guidelines. As we look to building a design system that extends and implements our brand guidelines, replacing with a more appropriate typeface is an important first step. This will be used as a foundation to build future experiences on, and iteratively create a site with consistent typesetting.
This work replaces Open Sans with Neue Haas Grotesk, delivered via Adobe Fonts. This work also includes:
_about.scss
file.Note that this work does not address:
Relevant tickets:
Many commits are prefixed with their respective ticket from the JIRA epic PW-133.
Stylesheets
string incremented.
Secrets
Documentation
Accessibility
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Stakeholder approval
Dependencies
YES | NO dependencies are updated
Code Reviewer
(not just this pull request message)