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

Jump to bottom

Fix issue 18571 - showcase details page titles #18705

Closed
wants to merge 2 commits into from
Closed

Fix issue 18571 - showcase details page titles #18705

wants to merge 2 commits into from

Conversation

mrkutly
Copy link
Contributor

@mrkutly mrkutly commented Oct 16, 2019 •
edited
Loading

Description

Adds titleTemplate="" prop to the Helmet component in components/showcase-details.js. Also adds defer={false} prop to the Helmet in views/showcase/index.js. There are multiple Helmet components being rendered at the same time, which was causing strange behavior for when the details page was a modal versus the full page.

For the React details page, you would sometimes get the title:
"React.js,: Showcase | GatsbyJS | GatsbyJS"

This PR fixes it so that it is now whether rendered as a modal or a full page:
"React.js: Showcase | GatsbyJS"

The titleTemplate="%s | GatsbyJS" prop from the top-level Helmet in site-metadata.js was interfering with the title rendering correctly in the showcase-details. Once that was fixed by overwriting it in the showcase-details.js Helmet, the defer={false} prop was needed to prevent the Helmet in views/showcase/index.js from flickering with the bad version of the title while transitioning between modal open and closed state.

Related Issues

Fixes 18571

@mrkutly mrkutly requested a review from a team as a code owner October 16, 2019 00:53
@muescha
Copy link
Contributor

muescha commented Oct 16, 2019

Is your PR is more a workaround?

@mrkutly
Copy link
Contributor Author

mrkutly commented Oct 18, 2019

Yes I suppose it is more a workaround than a good solution. I will try to find a better way to solve this that does not involve overriding the titleTemplate prop.

@mrkutly mrkutly closed this Oct 18, 2019
@mrkutly
Copy link
Contributor Author

mrkutly commented Oct 18, 2019 •
edited
Loading

@muescha I found what I believe is a much better solution. It is still a bit of a workaround, but it fixes the issue much more cleanly.

There is this open issue for React Helmet about the extra commas being added into the titles. The current workaround for this is to wrap the title text in a string inside {}. This removes the comma.

I'm currently setting the titleTemplate prop on the Helmet component in showcase-details.js to be "%s | GatsbyJS". This is because sometimes this is the only Helmet component on the page, so it needs the template. Before, this was being worked around by hardcoding " | GatsbyJS" into the title, which was causing the double "| GatsbyJS". By using titleTemplate, this will only render once.

This might be a good solution, but I'll see if I can figure out why the site metadata is different depending on how you arrive at a page.

@muescha
Copy link
Contributor

muescha commented Oct 18, 2019

Should we ask the @gatsbyjs/core team what's the best here to avoid double helmet

@mrkutly
Copy link
Contributor Author

mrkutly commented Oct 18, 2019 •
edited
Loading

@muescha I don't think having multiple helmets is a problem as it is supported by the react-helmet library. We will need multiple helmets to update the title as the user navigates around the page.

The issue was that the "| GatsbyJS" was hardcoded in one title tag but also set as the titleTemplate prop for a parent Helmet.

According to the react-helmet docs:

Nested or latter components will override duplicate changes:

<Parent>
    <Helmet>
        <title>My Title</title>
        <meta name="description" content="Helmet application" />
    </Helmet>

    <Child>
        <Helmet>
            <title>Nested Title</title>
            <meta name="description" content="Nested component" />
        </Helmet>
    </Child>
</Parent>
outputs:

outputs

<head>
    <title>Nested Title</title>
    <meta name="description" content="Nested component">
</head>

So we just need to make sure that we use titleTemplate for the top-level Helmet components (whichever those are at any time) and that we do not hardcode the template into the title itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extra comma and " | Gatsby" in showcase details title
2 participants
@mrkutly @muescha