Fix bugs on Moment.js lazy loading #894
Conversation
davilima6
commented
Feb 16, 2019
davilima6
commented
- Avoid leaking lazyLoadMomentLocale to global namespace
- Fix bug by not lower casing mockup-i18n's currentLanguage
- Fix bug on IE11 due to lack of support for 'includes'
|
@davilima6 thanks for creating this Pull Request and help improve Plone! To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass. Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating: With this simple comment all the jobs will be started automatically. Happy hacking! |
|
@jenkins-plone-org please run jobs |
- Avoid leaking lazyLoadMomentLocale to global namespace - Fix not incorrect formatting of mockup-i18n's currentLanguage - Fix widget breakage on IE11 due to lack of support for 'includes'
davilima6
force-pushed
the
momentjsLazyLoadLocales
branch
from
0973177
to
4211674
Compare
|
Sometimes things are more complex than they seem, right? But now it really looks fine. |
mockup/patterns/moment/pattern.js
Outdated
| 'mr ms-my ms my nb ne nl nn pl pt-br pt ro ru se si sk sl sq sr-cyrl ' + | ||
| 'sr sv sw ta te th tl-ph tlh tr tzl tzm-latn tzm uk uz vi zh-cn zh-tw'; | ||
|
|
||
| lazyLoadMomentLocale(currentLanguage); |
There was a problem hiding this comment.
I would move this invocation after the function is defined and would call it without an argument (currentLanguage can be evaluated in the scope of lazyLoadMomentLocale, see comment above).
Not a merge blocker.
There was a problem hiding this comment.
You're right. I'll change that.
About the function declaration since it's hoisted should be fine but I'll double check, probably your suggestion makes things more readable.
There was a problem hiding this comment.
I pushed the 2 suggested changes but I must confess I wasn't too sure about the 2nd one. It seems to me not relying on local scope would be safer, for instance the function could become easier to test and also move around, for example, if we wanted it to go in a moment-locale pattern (i don't like bloating the pattern file, in this case it was OK-ish because it's little code but I'd prefer if it looked more similar to the others: define wrapper + extend Base + return extended object. yet I preferred to tackle that in a next PR)
|
@jenkins-plone-org please run jobs |
|
@ale-rt, no pain! Just asking for suggestions on how to improve it. I'll merge then, thank you! |
|
Test failure unrelated, I am merging this one |
