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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d064bee
add meta tags for SEO
fabiosantoscode Apr 10, 2020
c16edb6
Merge branch 'master' into seo-meta
rogermparent Apr 23, 2020
623b59e
Add aria-label props to improve SEO
rogermparent Apr 24, 2020
cde7cab
Make meta image sources absolute and re-add old sizes
rogermparent Apr 27, 2020
7efc80c
Change SEO image prop from string to fluid image
rogermparent Apr 27, 2020
562e4ff
Revert favicons to original state
rogermparent Apr 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/HamburgerIcon/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import React from 'react'

import styles from './styles.module.css'

interface IHamburgetProps {
interface IHamburgerProps {
opened?: boolean
}

const HamburgerIcon: React.FC<IHamburgetProps> = ({ opened }) => (
const HamburgerIcon: React.FC<IHamburgerProps> = ({ opened }) => (
<div className={cn(styles.wrapper, opened && styles.opened)}>
<div className={cn(styles.line, styles.first)} />
<div className={cn(styles.line, styles.second)} />
Expand Down
3 changes: 2 additions & 1 deletion src/components/HamburgerMenu/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,14 @@ const HamburgerMenu: React.FC = () => {
className={styles.toggleButton}
onClick={toggleMobileMenu}
onKeyDown={openOnEnterKey}
aria-label="Toggle Mobile Menu"
>
<HamburgerIcon opened={isOpened} />
</button>

<div className={cn(styles.wrapper, isOpened && styles.opened)}>
<div className={styles.logoRow}>
<Link href="/" className={styles.logo}>
<Link href="/" className={styles.logo} aria-label="Home">
<LogoSVG />
</Link>
</div>
Expand Down
7 changes: 6 additions & 1 deletion src/components/LayoutHeader/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ const LayoutHeader: React.FC<Required<ILayoutModifiable>> = ({ modifiers }) => {
className={cn(styles.container, collapsed && styles.collapsed)}
wide={includes(modifiers, LayoutModifiers.Wide)}
>
<Link href="/" className={styles.logoLink} title="DVC">
<Link
href="/"
className={styles.logoLink}
title="DVC"
aria-label="DVC"
>
<LogoSVG className={styles.logo} />
</Link>
<Nav />
Expand Down
59 changes: 40 additions & 19 deletions src/components/Page/DefaultSEO/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,26 @@
import React from 'react'
import ReactHelmet from 'react-helmet'
import Helmet from 'react-helmet'

import { MetaProps } from '../../SEO'
import { IPaginatorPageInfo } from '../../Paginator'
import getSiteMeta from '../../../queries/siteMeta'

interface IDefaultSEOProps {
pageInfo?: IPaginatorPageInfo
pathname: string
}

const DefaultSEO: React.FC<IDefaultSEOProps> = ({ pageInfo }) => {
const metaImage = {
src: '/social-share.png',
width: '1200',
height: '630'
}

const DefaultSEO: React.FC<IDefaultSEOProps> = ({ pathname }) => {
const siteMeta = getSiteMeta()
const metaTitle =
pageInfo && pageInfo.currentPage > 1
? `${siteMeta.title} page ${pageInfo.currentPage}`
: siteMeta.title
const siteUrl = siteMeta.siteUrl
const metaTitle = siteMeta.title
const metaDescription = siteMeta.description
const metaKeywords = siteMeta.keywords
const metaImage = '/social-share.png'
const fullUrl = siteUrl + pathname

const meta: MetaProps[] = [
{
Expand All @@ -28,6 +31,10 @@ const DefaultSEO: React.FC<IDefaultSEOProps> = ({ pageInfo }) => {
name: 'keywords',
content: metaKeywords
},
{
property: 'og:url',
content: fullUrl
},
{
property: 'og:title',
content: metaTitle
Expand All @@ -42,11 +49,23 @@ const DefaultSEO: React.FC<IDefaultSEOProps> = ({ pageInfo }) => {
},
{
property: 'og:image',
content: metaImage
content: siteUrl + metaImage.src
},
{
property: 'og:image:width',
content: metaImage.width
},
{
property: 'og:image:height',
content: metaImage.height
},
{
property: 'og:image:secure_url',
content: metaImage
content: siteUrl + metaImage.src
},
{
name: 'twitter:site',
content: '@dvcORG'
shcheklein marked this conversation as resolved.
Show resolved Hide resolved
},
{
name: 'twitter:card',
Expand All @@ -62,12 +81,12 @@ const DefaultSEO: React.FC<IDefaultSEOProps> = ({ pageInfo }) => {
},
{
name: 'twitter:image',
content: encodeURI(`${siteMeta.siteUrl}${metaImage}`)
content: encodeURI(`${siteUrl}${metaImage.src}`)
}
]

return (
<ReactHelmet
<Helmet
htmlAttributes={{
lang: 'en'
}}
Expand All @@ -76,21 +95,23 @@ const DefaultSEO: React.FC<IDefaultSEOProps> = ({ pageInfo }) => {
meta={meta}
link={[
{
rel: 'shortcut icon',
rel: 'icon',
type: 'image/vnd.microsoft.icon',
href: '/favicon.ico'
href: siteUrl + '/favicon.ico'
},
{
rel: 'icon',
type: 'image/png',
href: '/favicon-32x32.png',
sizes: '32x32'
href: siteUrl + '/favicon-32x32.png'
},
{
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 •
edited
Loading

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 •
edited
Loading

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.

},
{
rel: 'canonical',
href: fullUrl
}
]}
/>
Expand Down
2 changes: 1 addition & 1 deletion src/components/Page/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const Page: React.FC<IPageProps> = props => {

return (
<>
<DefaultSEO />
<DefaultSEO pathname={props.location.pathname} />
<LayoutComponent {...props} />
</>
)
Expand Down
22 changes: 18 additions & 4 deletions src/components/SEO/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ interface ISEOProps {
defaultMetaTitle?: boolean
description?: string
keywords?: string
image?: string
image?: {
src: string
presentationWidth: number
presentationHeight: number
}
meta?: MetaProps[]
pageInfo?: IPaginatorPageInfo
children?: React.ReactNode
Expand Down Expand Up @@ -72,18 +76,28 @@ const SEO: React.FC<ISEOProps> = ({
}

if (image) {
const imageUrl = siteMeta.siteUrl + image.src

prebuildMeta.push(
{
property: 'og:image',
content: image
content: imageUrl
},
{
property: 'og:image:secure_url',
content: image
content: imageUrl
},
{
property: 'og:image:width',
content: String(image.presentationWidth)
},
{
property: 'og:image:height',
content: String(image.presentationHeight)
},
{
name: 'twitter:image',
content: encodeURI(`${siteMeta.siteUrl}${image}`)
content: imageUrl
}
)
}
Expand Down
9 changes: 3 additions & 6 deletions src/templates/blog-post.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import SEO from '../components/SEO'
import Post from '../components/Blog/Post'

interface IFluidObject extends FluidObject {
presentationWidth?: number
presentationWidth: number
presentationHeight: number
}

export interface IGatsbyImageProps extends GatsbyImageProps {
Expand Down Expand Up @@ -51,10 +52,6 @@ interface IBlogPostPageProps {
data: {
blogPost: IBlogPostData
}
pageContext: {
next: IBlogPostData
previous: IBlogPostData
}
}

const BlogPostPage: React.FC<IBlogPostPageProps> = ({ data }) => {
Expand All @@ -66,7 +63,7 @@ const BlogPostPage: React.FC<IBlogPostPageProps> = ({ data }) => {
<SEO
title={title}
description={description}
image={picture && picture.fluid.src}
image={picture && picture.fluid}
/>
<Post {...post} />
</>
Expand Down
2 changes: 2 additions & 0 deletions static/robots.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
Sitemap: https://blog.dvc.org/sitemap.xml

User-agent: *
Disallow: