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

Use default locale's custom path if not defined for a locale #354

Merged

Conversation

jeanphilippeds
Copy link

Hello,

Here is a PR to use default locale's custom path when a custom path is not defined for a locale.
It can help if you have a lot of locales to manage and only want a custom path for a few of them.

@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #354 into 6.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            6.x   #354   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         2      2           
  Lines         6      6           
===================================
  Hits          6      6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95af03f...012345a. Read the comment docs.

* @param {Object} pages Pages options from module's configuration
* @param {Array} locales Locale from module's configuration
* @param {String} pagesDir Pages dir from Nuxt's configuration
* @param {String} defaultLocale Pages dir from Nuxt's configuration

Choose a reason for hiding this comment

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

the description seems to have been copy-pasted from the previous line 😕

Copy link
Author

Choose a reason for hiding this comment

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

fixed 😜

@jeanphilippeds jeanphilippeds force-pushed the use-default-locale-custom-paths branch from cc4e8a1 to e315432 Compare July 10, 2019 18:58
@jeanphilippeds jeanphilippeds force-pushed the use-default-locale-custom-paths branch from e315432 to 814f4a5 Compare July 11, 2019 11:17
@paulgv paulgv changed the base branch from master to 6.x July 13, 2019 15:47
Copy link
Collaborator

@paulgv paulgv left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jeanphilippeds.

This change makes sense to me but it should be considered as a breaking change. I've changed the base branch to 6.x and we'll release this in the next major version.

I left one small comment, could you address it before we merge this?

src/helpers/utils.js Outdated Show resolved Hide resolved
Co-Authored-By: Paul Gascou-Vaillancourt <paul.gascvail@gmail.com>
@rchl
Copy link
Collaborator

rchl commented Nov 28, 2020

@jeanphilippeds I know this is an old PR but I've just stumbled upon the behavior introduced by this change and I do wonder how it does make sense...

When we have

  • two locales: en and fr
  • the default locale is fr
  • we have an about.vue page for which we set custom path:
{
  'fr': '/about-fr'
}

With this change here the English locale will also use the about-fr name for the about page. This doesn't seem right to me.

Without this change, the en locale would use the about name.

Maybe the intention here was to handle a case like for example:

  • three locales: en, fr, es
  • the default locale is en
  • only fr locale has overridden path:
{
  'fr': '/about-fr'
}

And then both the en and es locales default to about. That makes sense but that would also happen without this change. So I don't really get it.

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.

None yet

4 participants