Added canonical link to html head #1102
Conversation
| @@ -166,13 +167,19 @@ const AppRouts = (props: { | |||
| isUserAuthenticated={props.authenticationStore.isUserAuthenticated} | |||
| appStore={props.appStore} | |||
| path={PAGE_ROUTE.GENE} | |||
| canonicalProps={{ | |||
| __canonicalTypeName: 'location', | |||
There was a problem hiding this comment.
to denote it's only need to help typescript with type hinting.
It's a convention I stole from a graphql library I used for CTDataHub.
There was a problem hiding this comment.
Could you help me with "help typescript with type hinting"? We don't use the convention anywhere in the project, seems a bit odd to me to add only for this prop here. But want to know whether we should adopt across the board.
There was a problem hiding this comment.
Basically I'm trying to know what object I'm looking at. If I use the "typeof" or "instanceof" keywords it will only tell me that it's an object. I could make a class with instanceof, but that seems like overkill so what I'm doing instead is telling typescript that there can only be a handful of hardcoded strings which are valid in the "canonicalTypeName" field. Since each string is unique to each object type, performing an equality check tells typescript exactly which type you're looking at.
There was a problem hiding this comment.
Maybe I could use the "in" operator instead
| : canonicalProps.__canonicalTypeName === 'function' | ||
| ? canonicalProps.getCanonicalEndpoint(routeProps, removedPropsUrl) | ||
| : canonicalProps.__canonicalTypeName === 'location-without-url-params' |
There was a problem hiding this comment.
They aren't used anywhere in the PR.
I added them because I was going to used them, but refactored them away.
function allows you to hand a callback to the component to allow you to build the canonical url based on the information provided by react router.
location-without-url-params strips the url params from the url so for example this url
/gene/:hugoSymbol/:alteration
becomes
/gene
I can remove it if you think it'll make things too confusing.
There was a problem hiding this comment.
It's ok without removing. But can we please add the comment above in the code?
|
|
||
| linkNode.setAttribute('id', 'canonical'); | ||
| linkNode.setAttribute('rel', 'canonical'); | ||
| linkNode.setAttribute('href', `${location.host}${canonicalEndpoint}`); |
There was a problem hiding this comment.
I see wikipedia does include the protocol in their canonical link tag. Is that optional?
There was a problem hiding this comment.
There was a problem hiding this comment.
I meant whether we should include http or https
There was a problem hiding this comment.
I need to get better a reading. 😄
Yeah we need it. Good catch!
| document.head.appendChild(linkNode); | ||
|
|
||
| return () => { | ||
| document.head.removeChild(linkNode); |
There was a problem hiding this comment.
because I didn't add a canonical link for every route so I don't want to confuse the google crawler by having an incorrect link
There was a problem hiding this comment.
Oh right, nvm. I thought the page is reloaded on every redirect, but we are SPA. In that case, do we need to look for the canonical tag before appending? Or is the dom is start enough to avoid dup?
There was a problem hiding this comment.
Avoid duplication and to remove the link tag all together if we didn't specify needing one.
There was a problem hiding this comment.
Sorry how is it doing what you said above?
There was a problem hiding this comment.
It always deletes the link on a route change, but only adds it back when you specify in the OncokbRoute component that you want a canonical link.
| <> | ||
| {React.createElement(component!, props)} | ||
|
|
||
| <CanonicalLink routeProps={props} canonicalProps={canonicalProps} /> |
There was a problem hiding this comment.
Why the canonical link is a component?
There was a problem hiding this comment.
So I can hook into react lifetime and also have access to routeProps from react-router
jfkonecn
force-pushed
the
google-search-fix
branch
from
3e16fe2
to
6635fca
Compare
fixes oncokb/oncokb-pipeline#321