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

Add trailing slash to internal links option #32

Merged

Conversation

lemoswilson
Copy link

On HFO all canonical urls have trailing slash, and a bunch of links on the CMS don't have the trailing slash, so a lot of the links in the pages are actually not the canonical version, and thus are not the URLs indexed. This means that we are linking towards pages that are not true end pages in Google’s eyes.

To fix this I propose adding a internalTrailingSlash prop to the smart-link component, if this prop is true, then the link will automatically add a trailing slash to the URL.

index.coffee Outdated
Comment on lines 101 to 103
url = new URL to
url.pathname += if url.pathname.endsWith "/" then "" else "/"
url.toString()
Copy link
Member

Choose a reason for hiding this comment

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

How about this instead. It maybe computationally cheaper (don't need to construct a URL instance) and is more backwards compatible (since endsWidth isn't available in IE11).

Suggested change
url = new URL to
url.pathname += if url.pathname.endsWith "/" then "" else "/"
url.toString()
if to.match /\/$/ then to else to + '/'

Copy link
Author

Choose a reason for hiding this comment

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

replacing the URL object approach with a direct string manipulation had some problems when dealing with urls such as "https://www.happyfamilyorganics.com/learning-center/all?postCategory=postnatal". Therefore I dropped the endsWith method for the regex match, but kept the URL construct.

props: to: String
props:
to: String # The URL gets passed here
internalTrailingSlash: Boolean # Optionally add trailing slash if is internal link
Copy link
Member

@weotch weotch Apr 18, 2022

Choose a reason for hiding this comment

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

This makes sense, but it does seem like pain to set everywhere.

You see how there is this concept of settings in this package?

# Default settings
settings =
addBlankToExternal: false
internalUrls: []
sameWindowUrls: []
internalHosts: []
externalPaths: []

I think you should make addTrailingSlashToInternal as a setting here. Then, because the nuxt module of this project automatically relays it's setting, we should be able to set addTrailingSlashToInternal in the anchorParser settings of a project's nuxt.config.

However, we still have the problem of how do we get smart-link to read those settings. To solve this, we'll used Nuxt's publicRuntimeConfig feature.

You should edit the nuxt/module.js file and add

this.options.publicRuntimeConfig.anchorParser = options

This will relay the module options for accessing in $config in all components. I talk about this approach here with regards to setPublicDefaultOptions.

Because this component is functional, we can't access this. But we can access parent and, from there, we can get at $config.

So you want to edit the render function to destructure parent. Then you would add something like:

if internalTrailingSlash or parent?.$config?.anchorParser?.addTrailingSlashToInternal
then makeRouterPath addTrailingSlash to
else makeRouterPath to

Thus, we enable internalTrailingSlash globally on a project from the nuxt.config.

I did a little POC of accessing publicRuntimeConfig from a functional component here: https://codesandbox.io/s/jolly-fast-w8zngq?file=/components/fake-smart-link.coffee

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, looks like a good idea, definitely easier to enable this setting globally by following this approach. Thanks for the feedback.

@weotch weotch merged commit 0bea2af into BKWLD:master Jun 24, 2022
@weotch
Copy link
Member

weotch commented Jun 24, 2022

Sorry it took me a while to loop back on this, but i just merged and tagged it

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.

None yet

2 participants