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

sessionStorage support #215

Merged
merged 7 commits into from
Jun 15, 2020
Merged

sessionStorage support #215

merged 7 commits into from
Jun 15, 2020

Conversation

udf2457
Copy link
Contributor

@udf2457 udf2457 commented Jun 14, 2020

Checklist

  • [ X ] only relevant code is changed (make a diff before you submit the PR)
  • [ X ] run tests npm run test
  • [ X ] tests are included
  • documentation is changed or added

Pull request for sessionStorage. I'm still a bit of a JS newbie, so I hope they're ok !

> [email protected] pretest /Users/udf2457/i18next-browser-languageDetector
> npm run test:typescript && npm run test:typescript:noninterop


> [email protected] test:typescript /Users/udf2457/i18next-browser-languageDetector
> tslint --project tsconfig.json


> [email protected] test:typescript:noninterop /Users/udf2457/i18next-browser-languageDetector
> tslint --project tsconfig.nonEsModuleInterop.json


> [email protected] test /Users/udf2457/i18next-browser-languageDetector
> npm run build && mocha test -R spec --exit


> [email protected] build /Users/udf2457/i18next-browser-languageDetector
> rimraf dist && rollup -c && cpy "./dist/umd/*.js" ./


./src/index.js → ./dist/cjs/i18nextBrowserLanguageDetector.js...
Browserslist: caniuse-lite is outdated. Please run next command `npm update`
created ./dist/cjs/i18nextBrowserLanguageDetector.js in 491ms

./src/index.js → ./dist/esm/i18nextBrowserLanguageDetector.js...
created ./dist/esm/i18nextBrowserLanguageDetector.js in 202ms

./src/index.js → dist/umd/i18nextBrowserLanguageDetector.js...
created dist/umd/i18nextBrowserLanguageDetector.js in 142ms

./src/index.js → dist/umd/i18nextBrowserLanguageDetector.min.js...
created dist/umd/i18nextBrowserLanguageDetector.min.js in 386ms


  language detector
    cookie
      ✓ detect
      ✓ cacheUserLanguage
    path
      ✓ detect
    querystring
      ✓ detect


  4 passing (6ms)

@adrai
Copy link
Member

adrai commented Jun 14, 2020

Can you update the Readme too?

@udf2457
Copy link
Contributor Author

udf2457 commented Jun 15, 2020

Do you want me to close & reopen the PR or will you just pull the updated readme from the fork?

Sorry, don't know my way around the more edge-case corners of github. If there's a better way, let me know.

@adrai adrai merged commit 4fb57d5 into i18next:master Jun 15, 2020
@adrai
Copy link
Member

adrai commented Jun 15, 2020

released in v4.3.0

@khownz
Copy link
Contributor

khownz commented Aug 13, 2020

@udf2457 I've noticed that it only appears to be working when you add lookupsessionStorage to the options.

  1. This should probably be lookupSessionStorage (capital S), to match the other prop names.
  2. localStorage seems to be working with a default i18nextLng when it is added to the order array and there is no lookupLocalStorage set. We should probably match this behaviour.

@adrai
Copy link
Member

adrai commented Aug 13, 2020

I think this is intended, because it is not added by default: https://github.com/i18next/i18next-browser-languageDetector/blob/master/src/index.js#L16
@udf2457 what is your opinion?

@udf2457
Copy link
Contributor Author

udf2457 commented Aug 13, 2020

@adrai I'd love to have an opinion, but unfortunately I've moved on to other work and have not spent much time with i18next in recent months.

My guess @adrai is you are probably right in your observation. I'm not exactly a JS guru, so if this relates to the code you pulled in from me, it is quite possible I omited something.

P.S. Whilst I'm here, I agree with @khownz, consistency is good. If you're going to use camel-case, then use it everywhere.

@adrai
Copy link
Member

adrai commented Aug 13, 2020

@khownz do you like to submit a PR?

@khownz
Copy link
Contributor

khownz commented Aug 13, 2020

@adrai will do!

  1. Change lookupSessionStorage to be camelCased
  2. Add a default for lookupSessionStorage
  3. Update readme "Detector Options" section

@khownz
Copy link
Contributor

khownz commented Aug 13, 2020

@adrai done: #221

@adrai
Copy link
Member

adrai commented Aug 13, 2020

fyi, released in v6.0.0

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.

3 participants