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

SEO Improvements #1183

Merged
merged 6 commits into from
Apr 28, 2020
Merged

SEO Improvements #1183

merged 6 commits into from
Apr 28, 2020

Conversation

rogermparent
Copy link
Contributor

This is my continuation of the SEO PR by @fabiosantoscode.

I've merged master, removed a deprecated SFC typedef, and added aria-label props to a few buttons and links on the homepage that boost its Lighthouse accessibility score by around 10 points on my local tests.

We can squeeze the last few points of accesibility out by addressing the last flag it throws, which is some elements not having enough contrast between text and background. The first two I found were the mobile "Get Started" button and the gray-on-white GitHub text.

I can solve this by simply darkening colors and increasing contrast while sticking with the style, but this would easily be the biggest SEO change so I'm going to leave it out of this PR. The stuff here is invisible and pure SEO benefit.

fabiosantoscode and others added 3 commits April 10, 2020 20:42
I also removed the deprecated SFC type def.
This adds 10+ points to Lighthouse accessibility score.

Lighthouse sources:

https://web.dev/button-name
https://web.dev/link-name
@shcheklein
Copy link
Member

@rogermparent please, check also #1119 and #1118 . Especially for the meta fields one - it has some points to check.

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Looks great! 👍 just a few questions/comments.

@shcheklein shcheklein temporarily deployed to dvc-landing-seo-meta-hfy9y2iyf April 25, 2020 04:06 Inactive
Some external readers and parsers dislike root-relative links,
so this commit adds the site URL before all of them.

Also, I re-added the meta icons previously removed. I didn't find explicit
reasons for or against their inclusion, but considering the extra resource usage
is minimal I'm going to invoke chesterton's fence here and leave it as it was
before.
@rogermparent rogermparent temporarily deployed to dvc-landing-seo-meta-hfy9y2iyf April 27, 2020 20:47 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented Apr 27, 2020

Looks like I need to clean up some TS issues that got past the linter before a test deploy will work. I'll get on that.

Edit: Done! Just had to change passed image string to the object it came from.

This is what the TS def and the component itself expects.
@rogermparent rogermparent temporarily deployed to dvc-landing-seo-meta-hfy9y2iyf April 27, 2020 21:03 Inactive
},
{
rel: 'icon',
type: 'image/png',
href: '/favicon-16x16.png',
sizes: '16x16'
href: siteUrl + '/favicon-32x32.png'
Copy link
Member

Choose a reason for hiding this comment

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

16x16? do we need more sizes? we need to do this if we understand why do we have them.

Copy link
Contributor Author

@rogermparent rogermparent Apr 27, 2020

Choose a reason for hiding this comment

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

Oops! Looks like I kinda messed up this manual re-add without a quick git diff to lean on.
I also just found this:
https://support.google.com/webmasters/answer/9290858?hl=en
Seems like the original change that removed them was made based on this info, which provides guidelines to have one favicon at 48x48 (or a larger multiple of that) and explicitly says not to provide a 16x16 icon.
It also says to use the rel='shortcut icon bit removed from before, so I'll re-add that.
I'll adapt the PR to this new info.

Copy link
Member

Choose a reason for hiding this comment

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

yep, that link makes sense, but I would not do SEO in isolation from other needs. meta tags exist not only for Google search page, also link sharing, browsers, embeds, etc, etc - that's why reviewing meta tags ticket ticket is relevant to this one.

Copy link
Contributor Author

@rogermparent rogermparent Apr 27, 2020

Choose a reason for hiding this comment

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

You're right, and the article itself says that "penalized" sites weren't downranked, simply not allowed to use their custom 16x16 icon.

This should mean that despite Google ignoring it and downscaling a larger image, having the 16x16 (and any other sizes) shouldn't cause any harm. I'll keep the old entries exactly how they are and re-add shortcut to the .ico entry as it seems like the "best" icon given it's currently the primary one and has transparency.

Making these favicons absolute is probably premature as well, since favicons work now. I now think these favicons should just be reverted to their original state, which I'll do next commit.

Copy link
Member

Choose a reason for hiding this comment

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

@rogermparent my only point is that this ticket + meta tags one requires a bit of research :) what are the best practices now? what images and for what should we include, etc, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get what you mean. I generally use Google Lighthouse as the source of truth when if comes to SEO because SEO is more or less for Google. Since Lighthouse doesn't flag having all our original icon sizes as a problem, I think it's best for this PR to keep them to remove later.
Since they were removed in the first commit from @fabiosantoscode, I'll tag him to ask the reason.
I think not removing any is the best for this PR so it can go through easier and fix the share card image issue, which this PR appears to do now from my tests of a new build on it with both the Facebook debugger and another at http://debug.iframely.com for good measure.

The opengraph spec shows that they expect urls to be absolute instead of root-relative, which is where I believe the issue with share card images comes from.

@rogermparent rogermparent temporarily deployed to dvc-landing-seo-meta-hfy9y2iyf April 27, 2020 22:06 Inactive
@rogermparent rogermparent requested a review from shcheklein April 27, 2020 23:41
@rogermparent
Copy link
Contributor Author

I think this PR is currently in a good place to be considered for merging (after a review of course) to fix the OpenGraph issue in prod, and any further SEO-focused changes can continue this or open another.

@shcheklein
Copy link
Member

@rogermparent sounds good! merging. ...

@shcheklein shcheklein merged commit 1dd6226 into iterative:master Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants