vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

datepicker locale support #3878

Merged
konrad merged 5 commits from PterX/frontend:main into main 2024-01-20 18:50:02 +00:00
Conversation 2 Commits 5 Files Changed 7 +30 -30
PterX commented 2024-01-09 17:59:24 +00:00
Contributor
Copy Link

Resolves #3877

Resolves https://kolaente.dev/vikunja/frontend/issues/3877
PterX added 1 commit 2024-01-09 17:59:28 +00:00
continuous-integration/drone/pr Build is passing Details
ffb020d060 datepicker locale support
frederick commented 2024-01-09 18:05:29 +00:00
Member
Copy Link

Hi PterX!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3878-main--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi PterX! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3878-main--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
konrad reviewed 2024-01-10 11:58:41 +00:00
src/components/date/datepickerWithRange.vue Outdated
@ -95,6 +97,8 @@ const props = defineProps({
// FIXME: This seems to always contain the default value - that breaks the picker
const weekStart = computed(() => authStore.settings.weekStart ?? 0)
const locLang = FlatpickrLanguages[TRANS_LOCALES[authStore.settings.language]]
konrad commented 2024-01-10 11:56:23 +00:00
Owner

Please add a check if the selected language exists in flatpickr with a fallback to en.

Please add a check if the selected language exists in flatpickr with a fallback to en.
PterX commented 2024-01-10 12:27:20 +00:00
Author
Contributor

Let me add something

Let me add something
PterX marked this conversation as resolved
src/components/date/datepickerWithRange.vue Outdated
@ -96,2 +98,4 @@
// FIXME: This seems to always contain the default value - that breaks the picker
const weekStart = computed(() => authStore.settings.weekStart ?? 0)
const locLang = FlatpickrLanguages[TRANS_LOCALES[authStore.settings.language]]
locLang.firstDayOfWeek = weekStart.value
konrad commented 2024-01-10 11:55:55 +00:00
Owner

Can you put this whole thing (with weekStart) in a utility function which takes an authStore as parameter?

Can you put this whole thing (with `weekStart`) in a utility function which takes an `authStore` as parameter?
PterX commented 2024-01-10 12:27:45 +00:00
Author
Contributor

I'll add it

I'll add it
PterX marked this conversation as resolved
src/i18n/index.ts Outdated
@ -23,6 +23,28 @@ export const SUPPORTED_LOCALES = {
'sl-SI': 'Slovenščina',
} as const
export const TRANS_LOCALES = {
konrad commented 2024-01-10 11:58:34 +00:00
Owner

This seems pretty redundant. Can't we use the first part of the country-language key of SUPPORTED_LOCALES?

This seems pretty redundant. Can't we use the first part of the country-language key of `SUPPORTED_LOCALES`?
PterX commented 2024-01-10 12:26:37 +00:00
Author
Contributor

It seems unlikely that vi-vn needs to use the second part. If there is Traditional Chinese later, it cannot be separated.

It seems unlikely that vi-vn needs to use the second part. If there is Traditional Chinese later, it cannot be separated.
konrad commented 2024-01-10 14:51:00 +00:00
Owner

Okay, but can't we use a logic switch if lang == 'vi-vn': return 'vn' else return lang.substr(0, 2)? That would reduce the maintenance overhead while still allowing flexibility.

Okay, but can't we use a logic switch `if lang == 'vi-vn': return 'vn' else return lang.substr(0, 2)`? That would reduce the maintenance overhead while still allowing flexibility.
PterX commented 2024-01-10 17:18:11 +00:00
Author
Contributor

i have update code ,please check again

i have update code ,please check again
PterX marked this conversation as resolved
PterX added 1 commit 2024-01-10 17:15:50 +00:00
continuous-integration/drone/pr Build is failing Details
122a85f316 update datepicker locale
konrad commented 2024-01-10 18:01:36 +00:00
Owner
Copy Link

Please fix the lint issue.

Please fix the lint issue.
konrad requested changes 2024-01-10 18:06:27 +00:00
src/components/date/datepickerWithRange.vue Outdated
@ -94,7 +93,6 @@ const props = defineProps({
})
// FIXME: This seems to always contain the default value - that breaks the picker
konrad commented 2024-01-10 18:03:06 +00:00
Owner

Please remove the FIXME since that's related to the weekStart const which is not present anymore.

Please remove the `FIXME` since that's related to the `weekStart` const which is not present anymore.
PterX marked this conversation as resolved
src/helpers/dayLocale.ts Outdated
@ -0,0 +2,4 @@
import FlatpickrLanguages from 'flatpickr/dist/l10n'
import type { CustomLocale } from 'flatpickr/dist/types/locale'
export function getFlatLanguage(): CustomLocale {
konrad commented 2024-01-10 18:04:50 +00:00
Owner

Why FlatLanguage and not FlatpickrLanguage?

Why `FlatLanguage` and not `FlatpickrLanguage`?
PterX marked this conversation as resolved
src/helpers/dayLocale.ts Outdated
@ -0,0 +5,4 @@
export function getFlatLanguage(): CustomLocale {
const authStore = useAuthStore()
const lang = authStore.settings.language
const entries = Object.entries(FlatpickrLanguages).filter(([langKey]) => {
konrad commented 2024-01-10 18:06:11 +00:00
Owner

Why are you filtering the available languages? This looks like an extra step with no real benefit.

Why are you filtering the available languages? This looks like an extra step with no real benefit.
PterX commented 2024-01-11 03:29:23 +00:00
Author
Contributor

This is not a simple filtering. Since the language field of flatpickr is different from that of vikunja, we will try to compare the name of flatpickr + "-" with the first half of vikunja's name to get the CustomLocale of flatpickr. like #3878 (comment)

This is not a simple filtering. Since the language field of flatpickr is different from that of vikunja, we will try to compare the name of flatpickr + "-" with the first half of vikunja's name to get the CustomLocale of flatpickr. like https://kolaente.dev/vikunja/frontend/pulls/3878#issuecomment-56828
konrad commented 2024-01-11 12:10:58 +00:00
Owner

But why filter it at all? Why not compute the flatpickr language index and check if it exists? That would be O(1). Currently you're looping over the whole list and then take the first entry, which is O(n).

But why filter it at all? Why not compute the flatpickr language index and check if it exists? That would be O(1). Currently you're looping over the whole list and then take the first entry, which is O(n).
PterX marked this conversation as resolved
PterX added 1 commit 2024-01-11 14:54:07 +00:00
continuous-integration/drone/pr Build is passing Details
b6388aa489 update datepicker locale
PterX force-pushed main from b6388aa489 to 308ad23674 2024-01-12 06:41:20 +00:00 Compare
PterX requested review from konrad 2024-01-15 14:12:31 +00:00
konrad requested changes 2024-01-15 15:20:14 +00:00
src/components/tasks/GanttChart.vue
@ -84,2 +86,2 @@
const dayjsLanguageLoading = ref(false)
// const dayjsLanguageLoading = useDayjsLanguageSync(dayjs)
// const dayjsLanguageLoading = ref(false)
const dayjsLanguageLoading = useDayjsLanguageSync(dayjs)
konrad commented 2024-01-15 15:05:11 +00:00
Owner

Why did you uncomment this?

Why did you uncomment this?
PterX commented 2024-01-19 16:31:16 +00:00
Author
Contributor

Because GanttChart relies on dayjs to set local, I opened it when I saw the implementation here, and it also took effect.

Because GanttChart relies on dayjs to set local, I opened it when I saw the implementation here, and it also took effect.
konrad commented 2024-01-19 22:17:39 +00:00
Owner

Okay, makes sense.

Okay, makes sense.
✅ 1
konrad marked this conversation as resolved
src/helpers/dayLocale.ts Outdated
@ -0,0 +6,4 @@
const authStore = useAuthStore()
const lang = authStore.settings.language
const langPair = lang.split('-')
let language
konrad commented 2024-01-15 15:16:23 +00:00
Owner

Please rewrite this to something like this:

let language = FlatpickrLanguages[lang === 'vi-vn' ? 'vn' : 'en']
if (langPair.length > 0 && typeof FlatpickrLanguages[langPair[0]) !== undefined) {
		language = FlatpickrLanguages[langPair[0] as key]
}
Please rewrite this to something like this: ``` let language = FlatpickrLanguages[lang === 'vi-vn' ? 'vn' : 'en'] if (langPair.length > 0 && typeof FlatpickrLanguages[langPair[0]) !== undefined) { language = FlatpickrLanguages[langPair[0] as key] } ```
PterX marked this conversation as resolved
src/helpers/dayLocale.ts Outdated
@ -0,0 +17,4 @@
const fkey = langPair[0] as key
language = FlatpickrLanguages[fkey]
}
language.firstDayOfWeek = authStore.settings.weekStart ?? 0
konrad commented 2024-01-15 15:06:25 +00:00
Owner

Please use the week start from the selected flatpickr locale if none was defined in the user settings.

Please use the week start from the selected flatpickr locale if none was defined in the user settings.
PterX marked this conversation as resolved
PterX added 1 commit 2024-01-19 16:53:44 +00:00
continuous-integration/drone/pr Build is passing Details
298ef83b2e update implement
PterX requested review from konrad 2024-01-19 16:54:46 +00:00
konrad requested changes 2024-01-19 22:17:53 +00:00
konrad left a comment
Owner

One last tiny nit, then we're ready to merge!

One last tiny nit, then we're ready to merge!
src/components/tasks/partials/defer-task.vue Outdated
@ -45,3 +45,3 @@
import TaskService from '@/services/task'
import type {ITask} from '@/modelTypes/ITask'
import {useAuthStore} from '@/stores/auth'
import { getFlatpickrLanguage } from '@/helpers/dayLocale'
konrad commented 2024-01-19 22:15:12 +00:00
Owner

Can you rename the file to flatpickrLanguage instead of dayLocale? Makes it clearer from the filename what to expect.

Can you rename the file to `flatpickrLanguage` instead of `dayLocale`? Makes it clearer from the filename what to expect.
PterX marked this conversation as resolved
PterX added 1 commit 2024-01-20 12:59:01 +00:00
continuous-integration/drone/pr Build is passing Details
3d35cbe984 update util name
PterX requested review from konrad 2024-01-20 12:59:53 +00:00
konrad approved these changes 2024-01-20 18:49:36 +00:00
konrad left a comment
Owner

Thanks!

Thanks!
konrad merged commit 92f7d9ded5 into main 2024-01-20 18:50:02 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3878
No description provided.
Delete Branch "PterX/frontend:main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?