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

fix: support localePath with path input and customized routes #1088

Merged
merged 16 commits into from
Mar 7, 2021

Conversation

Energon7
Copy link

@Energon7 Energon7 commented Mar 4, 2021

 nuxtI18n: {
        paths: {
          en: '/about-us', 
          fr: '/a-propos',
          es: '/sobre'
      }
  }

Example:
Current locale is: fr.
Calling localePath('/about-us') --> returns /fr/a-propos;
or calling localePath('/en/about-us') --> returns /fr/a-propos;

tested when strategy is 'prefix_except_default'

@rchl
Copy link
Collaborator

rchl commented Mar 4, 2021

It doesn't look too good at the first sight to be honest.

At minimum you need to run the tests when developing.

@Energon7
Copy link
Author

Energon7 commented Mar 4, 2021

You will need to update tests. since, now translated links are returned

@rchl
Copy link
Collaborator

rchl commented Mar 4, 2021

Sorry but you need to make sure that your change and the tests are aligned. I'm sure you will find cases where your change is incorrect if you analyze the failing tests.

Also, localeRoute is supposed to return a route (or nothing) while with your change it sometimes returns a plain path. This is not in line with the API and is the first issue I see from a far perspective.

@Energon7
Copy link
Author

Energon7 commented Mar 4, 2021

Now check please, tests passed successfully.

@rchl
Copy link
Collaborator

rchl commented Mar 4, 2021

They did not pass yet.

I'll check more thoroughly when I have more time but please continue with it.

(Also you have a lot of style issues that will be reported once the tests pass. You can see those locally by running yarn lint)

@rchl
Copy link
Collaborator

rchl commented Mar 4, 2021

Also, I'd like a new test added that showcases a case that was previously failing but doesn't anymore with the fix.

@Energon7
Copy link
Author

Energon7 commented Mar 4, 2021

@rchl Your browser test is wrong. The error is related to this line: 75. localizedRoute.query = thisRoute.query.
If remove this line, all tests will pass successfully, but in this case, if you set the any query, it will be removed in translation.
For example:
setLocale('/about?page=1') --> /fr/a-propos

@rchl
Copy link
Collaborator

rchl commented Mar 4, 2021

The test asserts that loading a /nopage?a#h path doesn't change it to something else. And with your fix the URL is changed to /nopage?a#h?a which is obviously wrong.

The test is not wrong, there is a bug in your changes.

@Energon7
Copy link
Author

Energon7 commented Mar 4, 2021

Now tests passed successfully.

@Energon7 Energon7 changed the title fix localePath, translation custom paths fix: localePath, translation custom paths Mar 4, 2021
@rchl
Copy link
Collaborator

rchl commented Mar 5, 2021

I've also asked for a new test that tests the behavior that was fixed.

If you don't have time or don't know how to do it then this will have to wait until I have time.

This was referenced Mar 15, 2021
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

3 participants