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 language vs. locale #14784

Closed
wants to merge 1 commit into from
Closed

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Mar 21, 2019

  • Steps: set language to english and locale to german
  • Go to files app and upload a file
  • Expected "a few seconds ago" and on hover "21.03.2019 …"
  • Actually "vor einigen sekunden" and on hover "21.03.2019 …"
  • Injecting getLanguage instead of getLocale will make it "a few seconds ago" and on hover "2019/21/03 …"

I checked, and indeed moment JS does not allow to differenciate between language and locale. My "idea" would be, to get the localeData of OC.getLocale(), then set moment.locale() to OC.getLanguage() and then overwrite weekstart, ispm, format, ... with the localeData of getLocale, or the other way around.

Opinions @ChristophWurst @rullzer @MorrisJobke

_config: {…}
​​
abbr: "en"
​​
calendar: {…}
lastDay: "[Yesterday at] LT"
lastWeek: "[Last] dddd [at] LT"
nextDay: "[Tomorrow at] LT"
nextWeek: "dddd [at] LT"
sameDay: "[Today at] LT"
sameElse: "L"
<prototype>: Object { … }
​​
dayOfMonthOrdinalParse: /\d{1,2}(th|st|nd|rd)/
global: false
ignoreCase: false
lastIndex: 0
multiline: false
source: "\\d{1,2}(th|st|nd|rd)"
sticky: false
unicode: false
<prototype>: Object { … }
​​
invalidDate: "Invalid date"
​​
longDateFormat: {…}
L: "MM/DD/YYYY"
LL: "MMMM D, YYYY"
LLL: "MMMM D, YYYY h:mm A"
LLLL: "dddd, MMMM D, YYYY h:mm A"
LT: "h:mm A"
LTS: "h:mm:ss A"
<prototype>: Object { … }
​​
meridiemParse: /[ap]\.?m?\.?/i
global: false
ignoreCase: true
lastIndex: 0
multiline: false
source: "[ap]\\.?m?\\.?"
sticky: false
unicode: false
<prototype>: Object { … }
​​
months: (12) […]
0: "January"
1: "February"
2: "March"
3: "April"
4: "May"
5: "June"
6: "July"
7: "August"
8: "September"
9: "October"
10: "November"
11: "December"
length: 12
<prototype>: Array []
​​
monthsShort: (12) […]
0: "Jan"
1: "Feb"
2: "Mar"
3: "Apr"
4: "May"
5: "Jun"
6: "Jul"
7: "Aug"
8: "Sep"
9: "Oct"
10: "Nov"
11: "Dec"
length: 12
<prototype>: Array []
​​
ordinal: function ordinal()​​
relativeTime: Object { future: "in %s", past: "%s ago", s: "a few seconds", … }
​​
week: {…}
dow: 0
doy: 6
<prototype>: Object { … }
​​
weekdays: (7) […]
0: "Sunday"
1: "Monday"
2: "Tuesday"
3: "Wednesday"
4: "Thursday"
5: "Friday"
6: "Saturday"
length: 7
<prototype>: Array []
​​
weekdaysMin: (7) […]
0: "Su"
1: "Mo"
​​​2: "Tu"
​​​3: "We"
​​​4: "Th"
​​​5: "Fr"
​​​6: "Sa"
​​​length: 7
​​​<prototype>: Array []
​​weekdaysShort: (7) […]
​​​0: "Sun"
​​​1: "Mon"
​​​2: "Tue"
​​​3: "Wed"
​​​4: "Thu"
​​​5: "Fri"
​​​6: "Sat"
length: 7

fixes #15457

@nickvergessen nickvergessen added bug 2. developing Work in progress labels Mar 21, 2019
@nickvergessen nickvergessen added this to the Nextcloud 16 milestone Mar 21, 2019
@MorrisJobke MorrisJobke mentioned this pull request Mar 21, 2019
9 tasks
@ChristophWurst
Copy link
Member

I checked, and indeed moment JS does not allow to differenciate between language and locale. My "idea" would be, to get the localeData of OC.getLocale(), then set moment.locale() to OC.getLanguage() and then overwrite weekstart, ispm, format, ... with the localeData of getLocale, or the other way around.

While it sounds error prone I think this could actually work. I'm wondering if we're the first that have this problem or if there is a known workaround we could follow.

@rullzer rullzer mentioned this pull request Mar 26, 2019
9 tasks
@nickvergessen
Copy link
Member Author

I implemented hat solution and it seems to work quite well.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good and change makes sense otherwise

core/js/js.js Outdated
]);
return moment.localeData()._abbr;
}
localeData.parentLocale = getBestParentLocale();
Copy link
Member

Choose a reason for hiding this comment

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

I would just inline this function call

@rullzer rullzer force-pushed the bugfix/noid/language-vs-locale branch from aef375c to a221011 Compare April 3, 2019 13:19
@MorrisJobke
Copy link
Member

  {
    "message": "TypeError: undefined is not an object (evaluating 'OC.getLocale().split')\nat core/js/js.js:9:32995",
    "str": "TypeError: undefined is not an object (evaluating 'OC.getLocale().split')\nat core/js/js.js:9:32995"
  }

🙈

@MorrisJobke MorrisJobke mentioned this pull request Apr 3, 2019
2 tasks
@faily-bot
Copy link

faily-bot bot commented Apr 3, 2019

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 17483: failure

TESTS=jsunit

Show full log
PhantomJS 2.1.1 (Linux 0.0.0) LOG: 'JQMIGRATE: Migrate is installed, version 1.4.1'
PhantomJS 2.1.1 (Linux 0.0.0) ERROR
PhantomJS 2.1.1 (Linux 0.0.0): Executed 15 of 1077 ERROR (0.022 secs / 0.09 secs)

@nickvergessen
Copy link
Member Author

Passes locally. Maybe someone from @nextcloud/javascript can help?

This was referenced Apr 10, 2019
@nickvergessen
Copy link
Member Author

Rebased, lets see if time fixed it.

@MorrisJobke
Copy link
Member

Conflicts again :/

@nickvergessen nickvergessen force-pushed the bugfix/noid/language-vs-locale branch from eda0059 to 8cf9e04 Compare May 2, 2019 09:16
@nickvergessen
Copy link
Member Author

rebased

@nickvergessen
Copy link
Member Author

Still failing 🤷‍♂️

Signed-off-by: Joas Schilling <coding@schilljs.com>

Fix moment.js usage of language and locale

Signed-off-by: Joas Schilling <coding@schilljs.com>
@wiswedel
Copy link
Contributor

wiswedel commented Jan 2, 2020

@rullzer This looks kind of stale but keeps getting reported everywhere. Can this please be considered still for 18?

@skjnldsv
Copy link
Member

skjnldsv commented Apr 9, 2020

Status?

@nickvergessen
Copy link
Member Author

See first post

@skjnldsv
Copy link
Member

skjnldsv commented Apr 10, 2020

See first post

I read it, what about it?

@nickvergessen
Copy link
Member Author

Well i proposed an idea and now we need to say yes or no to this hack

@skjnldsv
Copy link
Member

While it sounds error prone I think this could actually work. I'm wondering if we're the first that have this problem or if there is a known workaround we could follow.

I'm in too!

@rullzer
Copy link
Member

rullzer commented Mar 30, 2021

I'm going to close this due to lack of activity.
Feel free to reopen if anybody wants to continue.

@ghost
Copy link

ghost commented Dec 9, 2021

This is still a problem in NC 22.2.0.

Is there a chance for progress on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timestamps are not displayed in the preferred language
6 participants