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: update defaults and generate sitemap.xml in dist directory #42

Merged
merged 6 commits into from
Feb 7, 2019
Merged

fix: update defaults and generate sitemap.xml in dist directory #42

merged 6 commits into from
Feb 7, 2019

Conversation

ricardogobbosouza
Copy link
Member

@ricardogobbosouza ricardogobbosouza commented Jan 16, 2019 •
edited
Loading

  • Update defaults options
  • Generate sitemap.xml directly on dist directory with the generate:done hook
  • Removed generate option

Resolves #27

@ricardogobbosouza ricardogobbosouza changed the title fix: update defaults fix: update defaults and generate sitemap.xml directly on dist directory Jan 22, 2019
@ricardogobbosouza ricardogobbosouza changed the title fix: update defaults and generate sitemap.xml directly on dist directory fix: update defaults and generate sitemap.xml directly on dist dir Jan 22, 2019
@ricardogobbosouza ricardogobbosouza changed the title fix: update defaults and generate sitemap.xml directly on dist dir fix: update defaults and generate sitemap.xml directly on dist directory Jan 22, 2019
@ricardogobbosouza ricardogobbosouza changed the title fix: update defaults and generate sitemap.xml directly on dist directory fix: update defaults and generate sitemap.xml in dist directory Jan 22, 2019
@ricardogobbosouza ricardogobbosouza changed the title fix: update defaults and generate sitemap.xml in dist directory feat: update defaults and generate sitemap.xml in dist directory Jan 22, 2019
@ricardogobbosouza ricardogobbosouza changed the title feat: update defaults and generate sitemap.xml in dist directory fix: update defaults and generate sitemap.xml in dist directory Jan 22, 2019
@ricardogobbosouza
Copy link
Member Author

Hi @NicoPennec
Could you take a look?

TheAlexLichter
TheAlexLichter reviewed Feb 1, 2019
View reviewed changes
src/index.js Outdated Show resolved Hide resolved
TheAlexLichter
TheAlexLichter suggested changes Feb 1, 2019
View reviewed changes
Copy link
Member

@TheAlexLichter TheAlexLichter left a comment

Choose a reason for hiding this comment

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

Nice PR! Left you a few comments

@NicoPennec NicoPennec self-requested a review February 1, 2019 21:57
TheAlexLichter
TheAlexLichter reviewed Feb 7, 2019
View reviewed changes
src/index.js

// TODO on generate process only and not on build process
if (options.generate) {
Copy link
Member

Choose a reason for hiding this comment

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

Last thing I promise! 😄

A notice for the user that generate isn't needed anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

@manniL great review!
Added notice with consola.warn

TheAlexLichter
TheAlexLichter reviewed Feb 7, 2019
View reviewed changes
src/index.js Outdated
})()
}
if (options.generate) {
consola.warn('The option `generate` isn\'t needed anymore')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
consola.warn('The option `generate` isn\'t needed anymore')
consola.warn('The option `sitemap.generate` isn\'t needed anymore')

Copy link
Member

Choose a reason for hiding this comment

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

So the user knows which prop it is exactly

Copy link
Member Author

Choose a reason for hiding this comment

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

Ready! 👍

@TheAlexLichter TheAlexLichter merged commit 2a0f0e3 into nuxt-community:dev Feb 7, 2019
@TheAlexLichter
Copy link
Member

many thanks @ricardogobbosouza ☺️

@NicoPennec
Copy link
Member

@ricardogobbosouza thank your for the nice feature, it's was in my ToDoList since a long time 👍

@manniL thanks for the reviews
But it's a breaking change from the current release.
The current release v0.2 is fully compliant with nuxt before hooks, so the commit message must be mention that point.

Apologize, I know that I'm very slow to review PRs currently (due to personal life), but please discuss with me before merge 😉

@TheAlexLichter
Copy link
Member

TheAlexLichter commented Feb 7, 2019 •
edited
Loading

@NicoPennec Sorry for that 😦 ! I tagged you for review some time ago 🙈

Regarding the change: You are right! No release has been made yet though.
Do you prefer to do a major release after all the other changes and only support Nuxt >= 2.x or would you like to stick with Nuxt 1.x compat?

PS: Both, @ricardogobbosouza and I are on https://discord.nuxtjs.org/ - Feel free to ping us there if you have time so we can communicate fast ☺️

@TheAlexLichter
Copy link
Member

@NicoPennec Short reminder that this was a breaking change (as you release v0.2.1 and v0.2.2)

NicoPennec pushed a commit that referenced this pull request Apr 15, 2019
@ricardogobbosouza
fix: automatically create sitemap in dist (#42)
43f8d90 
BREAKING CHANGE: usage of hook that require Nuxt >= 1.0
NicoPennec pushed a commit that referenced this pull request Apr 15, 2019
@ricardogobbosouza
fix: automatically create sitemap in dist (#42)
2767ccb 
BREAKING CHANGE: usage of hook that require Nuxt >= 1.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheAlexLichter TheAlexLichter TheAlexLichter approved these changes

@NicoPennec NicoPennec Awaiting requested review from NicoPennec

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

Successfully merging this pull request may close these issues.
3 participants
@ricardogobbosouza @TheAlexLichter @NicoPennec