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

Sitemap route exclusions #4770

Merged
merged 2 commits into from
Apr 14, 2018
Merged

Sitemap route exclusions #4770

merged 2 commits into from
Apr 14, 2018

Conversation

lightstrike
Copy link
Contributor

I would like to add support for excluding multiple routes from the sitemap via globbing, using minimatch like in #4538.

I also took a crack at addressing #4723 but it didn't work on my side. I haven't used pathPrefix before, maybe someone else can check?

@lightstrike lightstrike changed the title sitemap route exclusions and pathPrefix inclusion attempt Sitemap route exclusions Mar 30, 2018
@lightstrike lightstrike mentioned this pull request Mar 30, 2018
Closed
@gatsbybot
Copy link
Collaborator

gatsbybot commented Mar 30, 2018 •
edited
Loading

Deploy preview for using-drupal ready!

Built with commit 2097e1b

https://deploy-preview-4770--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Mar 30, 2018 •
edited
Loading

Deploy preview for gatsbygram ready!

Built with commit 2097e1b

https://deploy-preview-4770--gatsbygram.netlify.com

KyleAMathews
KyleAMathews suggested changes Apr 4, 2018
View reviewed changes
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Nice improvement, thanks!

packages/gatsby-plugin-sitemap/README.md Outdated
@@ -63,4 +66,5 @@ plugins: [
]
```

If you are using `pathPrefix` in `gatsby-config.js`, you must run `gatsby build --prefix-paths` to build a sitemap with the prefix included. See [path prefix docs](https://www.gatsbyjs.org/docs/path-prefix/#production-build).
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why was this added here? There's already a standalone doc page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was something that tripped me up when testing, largely since I haven't used or needed pathPrefix before.

I need to take another shot at the pathPrefix stuff before this ready to merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense. Please remove it though as it's not relevant to this plugin other than the plugin should support it.

@lightstrike
sitemap route exclusions and pathPrefix inclusion attempt
33ac9f3 
Signed-off-by: Geoffrey Sechter <geoffrey.sechter@gmail.com>
@lightstrike lightstrike force-pushed the sitemaps/pathPrefix branch from 0675fda to d3b77ba Compare April 11, 2018 02:53
@lightstrike
Copy link
Contributor Author

@KyleAMathews I took another look at the pathPrefix issue and described what I found in #4723.

I just removed the relevant pathPrefix code as it's not relevant to the sitemap exclusion logic and can be addressed separately.

@m-allanson
Copy link
Contributor

m-allanson commented Apr 12, 2018 •
edited
Loading

Thanks @lightstrike!

Gatsby recently added a requirement that contributors sign-off commits. Could you do git rebase --signoff and force push the amended branch?

Check out the new Developer Certificate of Origin docs or comment here if you've any questions.

Once that's in, this should be good to merge.

@pieh
Copy link
Contributor

pieh commented Apr 12, 2018

I have one small request - can you add minimatch to package.json? We should declare dependency and not rely on it being installed with other packages.

@lightstrike lightstrike force-pushed the sitemaps/pathPrefix branch 2 times, most recently from 4d8703d to a72315d Compare April 12, 2018 19:00
@lightstrike
functional pathPrefix logic with sitemap
2097e1b 
Signed-off-by: Geoffrey Sechter <geoffrey.sechter@gmail.com>
@lightstrike lightstrike force-pushed the sitemaps/pathPrefix branch from a72315d to 2097e1b Compare April 12, 2018 22:12
@lightstrike
Copy link
Contributor Author

@m-allanson and @pieh Done and done. 😄 I had some DCO issues and ended up running git filter-branch --msg-filter "cat - && echo && echo 'Signed-off-by: Geoffrey Sechter <geoffrey.sechter@gmail.com>'" HEAD~2..HEAD as recommended here.

I also got a working version of the pathPrefix working. withPrefix didn't work as expected as pathPrefix is always scoped to be / within that function. I wanted to use normalizePath but it's not exported and I didn't want to change gatsby-link in this PR, so I reused the logic. Let me know if it would be better to export normalizePath and use instead here.

@KyleAMathews KyleAMathews merged commit d8c9b55 into gatsbyjs:master Apr 14, 2018
@KyleAMathews
Copy link
Contributor

Thanks!

@lightstrike lightstrike mentioned this pull request Apr 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KyleAMathews KyleAMathews KyleAMathews approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lightstrike @gatsbybot @m-allanson @pieh @KyleAMathews