Skip to content
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

Get better Lighthouse score in a11y and performance #78

Merged
merged 4 commits into from
Dec 15, 2017
Merged

Get better Lighthouse score in a11y and performance #78

merged 4 commits into from
Dec 15, 2017

Conversation

myfrom
Copy link
Contributor

@myfrom myfrom commented Dec 12, 2017

I did a few things (all mentioned in the commits) to improve the Lighthouse score. There are a few differences between my tests and the test file from https://gci-leaders.netlify.com/ that were caused by my development environment and the fact that machine is fairly slow.

I also changed the output of /static from /out/static to the root of /out as we were discussing in #55

Here are test results before and after:
tests.zip

Related to #55, part of GCI task

Copy link
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why Lighthouse didn't complain about this, but the RocketChat logo can probably be shrunk to 72px.

<head>
<meta charset="utf-8">
<title>GCI Leaders</title>
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no">
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=5.0, user-scalable=yes">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maximum-scale=5.0, user-scalable=yes is not needed now.

Copy link
Member

@blazeu blazeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Favicon has white border, looks ugly, remove them.

Also, I don't know if this is accurate or not, but checked via https://realfavicongenerator.net/favicon_checker
image

favicon.ico should contain multiple sizes (in the same file).

Copy link
Member

@blazeu blazeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message should be in imperative present tense https://coala.io/commit
e.g. Improved ~> Improve

@jayvdb
Copy link
Member

jayvdb commented Dec 13, 2017

@myfrom , please undo your merge commit. We do proper rebases at coala. see http://coala.io/rebase .

Also please in future do not create pull requests from your 'master' branch. (but please do not close this PR; we'll get this one fixed & merged ; ask us for help on Zulip)

@myfrom
Copy link
Contributor Author

myfrom commented Dec 13, 2017

@andrewda @jayvdb @blazeu Fixed everything

@blazeu
Copy link
Member

blazeu commented Dec 13, 2017

If you turn the GCI logo on the website into png or vector with transparent background, it might reduce the size. Try it first.

@blazeu
Copy link
Member

blazeu commented Dec 13, 2017

Also, your branch is still outdated, you need to rebase. http://coala.io/rebase

@myfrom
Copy link
Contributor Author

myfrom commented Dec 13, 2017

Oh, sorry, should be OK now.

Copy link
Member

@blazeu blazeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the images are broken. See https://deploy-preview-78--gci-leaders.netlify.com/ (deploy/netlify on below)

And wiki icons are gone, you need to add them back, resize if needed, and add the alt

@myfrom
Copy link
Contributor Author

myfrom commented Dec 13, 2017

Oh, sorry, missed those new icons. I updated them and basically have everything working, I'll upload tomorrow.

@blazeu
But I have one big problem: someone made a script that instead of creating new data.json takes the current one. The current one is pointing to images in static which now are in root. How should I overcome this? Delete this script?

@andrewda
Copy link
Member

We should add a CACHE_VERSION and increment it whenever anything breaking occurs to the data. This will let us bust the cache if it becomes stale. I can do this in a few hours.

@blazeu
Copy link
Member

blazeu commented Dec 14, 2017

Why not just remove the image url from the JSON and then rely on the chat platform name.

Edit: @myfrom ^ You should do that

@andrewda
Copy link
Member

That would be one option, but adding a CACHE_VERSION would allow us to avoid this problem entirely in the future, such as if a new key were added or an existing one were taken away.

@blazeu
Copy link
Member

blazeu commented Dec 14, 2017

In the mean time, I'm gonna run npm run build:clean to ignore cache on netlify so that @myfrom can continue his work.

Imporve page accessibility (and Lighthouse score) by:
Adding `alt` attribute to images,
adding `lang="en"` to `<html>` tag,
making the page user-scaleable

Related to #55
Minify images to speed up page load time

Related to #55
Use Code-In logo as page favicon, also improving
best pracitses score in Lighthouse

Related to #55
Change the generator (and other files to retain compatibility)
to output contents of /static to /out instead of /out/static.
This allows files such as favicon or manifest to be placed
in root of the webpage.

Related to #55
@myfrom
Copy link
Contributor Author

myfrom commented Dec 14, 2017

Should be OK now if we run it through $ npm run build:clean

@blazeu
Copy link
Member

blazeu commented Dec 15, 2017

Okay.

After this one, I want you sent another PR to gh-pages to delete update data.(js|min.js|yml) to remove those old keys. (or anybody else or me).

Edit: ^ Done.

@blazeu
Copy link
Member

blazeu commented Dec 15, 2017

ack be803a9 f95ffaf cf4a35d 206a75c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants