search

cfpb / regulations-site

(DEPRECATED) Web interface for viewing U.S. federal regulations and other regulatory information
Other
28 stars 43 forks source link

Performance and accessibility tweaks #721

Closed ascott1 closed 9 years ago

ascott1 commented 9 years ago

This does the following:

@KimberlyMunoz

KimberlyMunoz commented 9 years ago

:fallen_leaf:

Reviewing this and your other pull request between meetings today.

KimberlyMunoz commented 9 years ago

Fonts

Tested this in IE 9, IE 10, IE 11, Chrome, and Safari. Here's what it looks like in IE9, 10 and Safari.

image

The one thing I want to flag is that there seems to be a standard for how we're embedding fonts. I went off the licensed-fonts.css* (which I didn't write, I just moved.) But what you're using seems to work, and uses the IE trick that most folks recommend. I would just change the font-weight lines in the .avenir-demi/bold mixins to fix in cases where the fonts don't load.

Performance

Old (upstream/master)

image

image

New

image

image

Tested using Justice.js and the test API. Testing on our internal API might be a better metric for what this will look like in the wild. Those numbers are a bit higher

ascott1 commented 9 years ago

Tested using Justice.js and the test API. Testing on our internal API might be a better metric for what this will look like in the wild. Those numbers are a bit higher

This is really cool. I wasn't familiar with Justice.js until now. You're right that the numbers are misleading since it's running locally, but it's a neat way to see that we're own the right path on my quest to get us under 1000ms in production.

Thanks for testing it so well! Mind merging when you get a chance?

ascott1 commented 9 years ago

The one thing I want to flag is that there seems to be a standard for how we're embedding fonts. I went off the licensed-fonts.css* (which I didn't write, I just moved.) But what you're using seems to work, and uses the IE trick that most folks recommend. I would just change the font-weight lines in the .avenir-demi/bold mixins to fix in cases where the fonts don't load.

Just noticed this. I'll dig into this later today and update the PR as I'd like it to match the new standard.

ascott1 commented 9 years ago

@KimberlyMunoz, I noticed that the CF licensed fonts file still points to the CDN. My understanding is that we'll soon be moving away from the CDN.

I matched the font weights in the mixins to those in the @font-face rules. I thought there was a reason I did it the other way originally, but I couldn't find anything to back that up.