-
Notifications
You must be signed in to change notification settings - Fork 62
chore: remove moment #1107
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
chore: remove moment #1107
Conversation
d864a61 to
faea871
Compare
Signed-off-by: Grigorii K. Shartsev <[email protected]>
faea871 to
b4b61ae
Compare
Signed-off-by: Grigorii K. Shartsev <[email protected]>
b4b61ae to
1046165
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to changes comment. Otherwise works fine
| import type { ClearAtPredefinedConfig, PredefinedUserStatus, UserStatus, UserStatusStatusType } from './userStatus.types.ts' | ||
| import moment from '@nextcloud/moment' | ||
| import { translate as t } from '@nextcloud/l10n' | ||
| import { t, getFirstDay } from '@nextcloud/l10n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screenshot from appData.userMetadata, this results in empty <html data-locale > (when was not set on personal settings). Maybe it should defaults to selected language; here to 'de'?
also custom window.firstDay from settings is not provided as in web, and always leads to default locale value:
- 'en' - Sunday
- 'de' - Monday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also custom
window.firstDayfrom settings is not provided as in web
Yes, this info is not available for clients, so, using locale data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screenshot from
appData.userMetadata, this results in empty<html data-locale >(when was not set on personal settings). Maybe it should defaults to selected language; here to 'de'?
I cannot reproduce this state... Even for new accounts it's en initially. How do you unset it?
Anyway, Nextcloud defaults it to 'en' in @nextcloud/l10n, so should be fixed there. But I'd fallback for the local device settings, not the language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
occ user:setting <userid> --delete core locale. Then in web it defaults to core.lang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: nextcloud-libraries/nextcloud-l10n#864
Signed-off-by: Grigorii K. Shartsev <[email protected]>
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| import { getCanonicalLocale } from '@nextcloud/l10n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late-thought opinion: getLanguage should be used for relative time (not absolute, that required locale format):
With 'en' language and 'de' locale we are geting human-readable represenation, but language of it won't match expected system:
| Locale \ Lang | 'en' | 'de' |
|---|---|---|
| 'en' | 1:23 PM, 'in 1 day' 🟢 | 13:23, 'in 1 day' |
| 'de' | 1:23 PM, 'in 1 Tag' | 13:23, 'in 1 Tag' 🟢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and this is an old discussion about using locale for durations.
Here it works the same way it works on the server.

☑️ Resolves
@nextcloud/moment:moment@nextcloud/l10nv2