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

feat: expose localeProperties property #1016

Merged
merged 9 commits into from
Jan 11, 2021
Merged

feat: expose localeProperties property #1016

merged 9 commits into from
Jan 11, 2021

Conversation

iifawzi
Copy link

@iifawzi iifawzi commented Jan 8, 2021

Hi, as mentioned here #916, @rchl decided to expose the current locale object, here's how i tackled it, i hope i did it in the correct way since this's my first contribution!

i think we can move the codeFromLocale function from seo-head to utils-common to use it here also, instead of duplication, if confirmed i will commit the change

@iifawzi iifawzi changed the title exposing localeProperties property exposing localeProperties property #918 Jan 8, 2021
@iifawzi iifawzi changed the title exposing localeProperties property #918 exposing localeProperties property Jan 8, 2021
@iifawzi iifawzi changed the title exposing localeProperties property exposing localeProperties property #916 Jan 8, 2021
@iifawzi iifawzi changed the title exposing localeProperties property #916 feat: exposing localeProperties property #916 Jan 8, 2021
@iifawzi iifawzi closed this Jan 8, 2021
@iifawzi iifawzi reopened this Jan 8, 2021
@rchl
Copy link
Collaborator

rchl commented Jan 9, 2021

  • The locales property can also be just an array of locales in which case your code will crash. If that's the case we can just set an empty object.
  • Please initialize the new property also around line 263
  • We'd need to add it to the documentation. In docs/content/en/api.md (and es version) for sure but ideally also mention it in the documentation of the locales option in options-reference.md.
  • At least a single test that checks that this property exists and is set to the correct value should be added.

@iifawzi
Copy link
Author

iifawzi commented Jan 10, 2021

Hi @rchl, i made all the mentioned points, changes are committed.
i added two tests to ensure that the property exists and is set to the correct value, when configuration is empty, and when array of locale objects is used.

docs/content/en/options-reference.md Outdated Show resolved Hide resolved
test/module.test.js Outdated Show resolved Hide resolved
@iifawzi iifawzi requested a review from rchl January 10, 2021 19:10
 - add TS types
 - set code property when using the array of codes form
 - klona the original locale object so that it's not possible to accidentally modify it
 - rephrase docs a bit
Copy link
Collaborator

@rchl rchl left a comment

Choose a reason for hiding this comment

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

Looks good from my side. I've done some small changes, please have a look and let me know if those look good to you.

@iifawzi
Copy link
Author

iifawzi commented Jan 10, 2021

looks awesome from my side, well done!

@rchl rchl changed the title feat: exposing localeProperties property #916 feat: expose localeProperties property Jan 11, 2021
@rchl rchl merged commit a9457a0 into nuxt-modules:master Jan 11, 2021
@iifawzi iifawzi deleted the expose-localeProperties-property branch January 11, 2021 12:08
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.

2 participants