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

DOP-4835: Fix meta robots not appearing due to missing page data #1169

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

rayangler
Copy link
Collaborator

@rayangler rayangler commented Jul 17, 2024

Stories/Links:

DOP-4835

Current Behavior:

Atlas prod - noindex does not appear in a meta tag

Staging Links:

Atlas staging - noindex appears in a meta tag.

Notes:

  • Re-introduces page data into Gatsby Head. This was missing due to page context no longer having page data.
  • Throws an error if page AST is missing from Gatsby Head again. This should hopefully stop the build with an error to emphasize that this is not expected.
  • Adjusts existing data sources for tests for the Head component to split between page context and data. This should better mock how data is being passed into the component.

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

Comment on lines -181 to +184
let mockPageContext = mockHeadPageContext;
it('renders the canonical tag from directive rather than pulling from snooty.toml', () => {
mockPageContext = mockCompleteEOLPageContext;
mockPageContext.page.children.push(metaCanonical);
render(<Head pageContext={mockPageContext} />);
const mockPageContext = { ...mockHeadPageContext.pageContext };
const mockData = { ...mockCompleteEOLPageContext.data };
mockData.page.ast.children.push(metaCanonical);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spreading the data instead of reusing local variable due to potential data leaking in between tests due to object modifications for the last 2 unit tests below. Spreading here will help keep copies of data separate to promote independent tests

@rayangler rayangler marked this pull request as ready for review July 17, 2024 16:36
@rayangler
Copy link
Collaborator Author

Realized that we can throw the error instead of just printing it out, so I changed that in the code. Might be more appropriate given the importance of having the AST in this component at the moment

Copy link
Collaborator

@caesarbell caesarbell left a comment

Choose a reason for hiding this comment

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

LGTM

@caesarbell
Copy link
Collaborator

Realized that we can throw the error instead of just printing it out, so I changed that in the code. Might be more appropriate given the importance of having the AST in this component at the moment

Good call, I was just about to ask about that, and if we could cause the build to fail since it seem important to have.

@rayangler rayangler merged commit 0452b0a into main Jul 17, 2024
4 checks passed
@rayangler rayangler deleted the DOP-4835 branch July 17, 2024 16:48
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