-
Notifications
You must be signed in to change notification settings - Fork 128
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
feat: closes #69 and & adds sitemap: false option to pages #70
base: dev
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #70 +/- ##
===================================
Coverage 100% 100%
===================================
Files 1 1
Lines 3 3
===================================
Hits 3 3 Continue to review full report at Codecov.
|
@NicoPennec |
Codecov Report
@@ Coverage Diff @@
## dev #70 +/- ##
===================================
Coverage 100% 100%
===================================
Files 1 1
Lines 3 3
===================================
Hits 3 3 Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## dev #70 +/- ##
===================================
Coverage 100% 100%
===================================
Files 1 1
Lines 3 3
===================================
Hits 3 3 Continue to review full report at Codecov.
|
this looks great! |
@pi0 @NicoPennec |
Thanks @SnirShechter for your proposal 🙏 Sorry for the delay, I will test and review your PR this week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make a benchmark before / after the addition of your feature?
I want to validate the right performance with the "acorn" parser during the generation of sitemap.
@@ -232,7 +236,7 @@ function flattenRoutes (router, path = '', routes = []) { | |||
flattenRoutes(r.children, path + r.path + '/', routes) | |||
} | |||
if (r.path !== '') { | |||
routes.push(path + r.path) | |||
routes.push({ ...r, url: path + r.path }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This update can create a breaking change in the usage of the filter
option of the sitemap module, no ?
if yes, your commit message must respect the conventional commit to bump the release version as "major" update by the release script
@@ -0,0 +1,4 @@ | |||
module.exports = { | |||
COMPONENT_OPTIONS_BLOCK: 'script', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any <page>.vue
file, it will always be a script
element.
Why create a constant?
@@ -0,0 +1,4 @@ | |||
module.exports = { | |||
COMPONENT_OPTIONS_BLOCK: 'script', | |||
COMPONENT_OPTIONS_KEY: 'sitemap' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be customisable, you can set that key in the sitemap configuration in nuxt.config.js
instead of create a constant.
const compiler = require('vue-template-compiler') | ||
// const { COMPONENT_OPTIONS_BLOCK, COMPONENT_OPTIONS_KEY } = require('constants') | ||
|
||
function extractComponentOptions (path, blockName, key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should rename the function and file from extractComponentOptions
to extractPageOptions
, because it is relative to the *.vue
file in the Pages
folder.
</template> | ||
<script> | ||
export default { | ||
sitemap:false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why limit the sitemap
option to a simple boolean for exclude / include a page from the sitemap?
Maybe you can support the following options in addition of sitemap: true
:
<script>
export default {
sitemap: {
changefreq: 'daily',
priority: 1,
lastmod: new Date(),
// ...
}
@NicoPennec |
just similar implementation with no extra deps #101 |
Closes #69 - adds data to routes in the filter function.
Also closes #c55 (for some reason the issue was not posted in github).