Skip to content

fix(VDatePicker): use start of week to calculate week numbers #21199

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

Merged
merged 7 commits into from
Apr 7, 2025

Conversation

J-Sek
Copy link
Contributor

@J-Sek J-Sek commented Apr 2, 2025

Description

fixes #20724
fixes #20490

  • ISO-ish 4-day requirement for the first week defaults to match en-US

Markup:

<template>
  <v-app style="background-color: rgba(var(--v-theme-on-surface), 0.2)">
    <v-container>
      <div class="d-flex justify-center align-center ga-3">
        <span class="opacity-70">locale:</span>
        <v-chip-group v-model="currentLocale">
          <v-chip filter label text="en" value="en"></v-chip>
          <v-chip filter label text="pl" value="pl"></v-chip>
          <v-chip filter label text="fr" value="fr"></v-chip>
          <v-chip filter label text="pt" value="pt"></v-chip>
        </v-chip-group>
      </div>
      <div class="d-flex justify-center align-center ga-3">
        <span class="opacity-70">startOfWeek:</span>
        <v-chip-group v-model="startOfWeek">
          <v-chip
            v-for="o in startOfWeekOptions"
            v-bind="o"
            :key="o.value"
            filter
            label
          ></v-chip>
        </v-chip-group>
      </div>
      <div class="d-flex flex-wrap justify-center ga-6">
        <v-locale-provider :locale="currentLocale">
          <v-date-picker
            v-for="d in 6"
            :key="d"
            :elevation="5"
            :first-day-of-week="startOfWeek"
            :model-value="new Date(2023 + d, 0, 1)"
            class="mt-3"
            hide-header
            show-adjacent-months
            show-week
          ></v-date-picker>
        </v-locale-provider>
      </div>
    </v-container>
  </v-app>
</template>

<script setup>
  import { ref } from 'vue'

  const currentLocale = ref('en')
  const startOfWeek = ref(undefined)
  const startOfWeekOptions = 'Sun|Mon|Tue|Wed|Thu|Fri|Sat'
    .split('|')
    .map((x, i) => ({ text: x, value: i }))
</script>

@J-Sek J-Sek requested a review from johnleider April 2, 2025 15:07
Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

Please make sure to check #20794 as I think some of the changes might effect your P.R.

@J-Sek
Copy link
Contributor Author

J-Sek commented Apr 2, 2025

If we decide to trust Intl.Locale we could rewrite firstDay = [ ... ] into weekInfo = [ ... ]
It is just a matter of mapping all the codes

[
    {code: 'US', intlCode: 'en-US'},
    {code: 'FR', intlCode: 'fr'},
    {code: 'PL', intlCode: 'pl'},
    // etc...
]
.map(({code, intlCode}) => {
    const {firstDay, minimalDays} = new Intl.Locale(intlCode).getWeekInfo()
    return `'${code}': { firstDay: ${firstDay}, minimalDays: ${minimalDays} },`
})
.join('\n')

@J-Sek
Copy link
Contributor Author

J-Sek commented Apr 2, 2025

Ok, this PR was wrong in the state when firstWeekSize was forced to 4 (ISO) while it is 1 in United States.
I am trying to align with #20794 so we can merge it together.
When generating weekInfo dictionary in place of firstDay map, there are sometimes multiple matches and the old value do not align. It would be great if we could resolve this quickly.

@J-Sek
Copy link
Contributor Author

J-Sek commented Apr 2, 2025

@johnleider this PR can be merged instead of #20794 once we resolve changes to firstDay

Code old Intl DateFns Day.js CLDR
AG 0 1 - - 0
BS 0 1 1/4 1 0
BT 0 1 or 0(dz) - - 0
BW 0 1 or 0(tn) - - 0
DJ 6 1 - - 6
DM 0 1 - - 0
MM 0 1 or 0(my) - - 0
MZ 0 1 - - 0
SM 1 0 - - 1
UM 0 1(en-UM) - - 0
VI 0 1 1/1 1 0
WS 0 1 or 0(sm) - - 0

Options:

  • revert back to original values
    • although BS and VI have mixed info nevermind, mapped to correct locale codes
  • accept all
  • pick and choose

@J-Sek J-Sek self-assigned this Apr 2, 2025
@J-Sek J-Sek added T: bug Functionality that does not work as intended/expected C: VDatePicker E: date i18n Internationalization issue labels Apr 2, 2025
@J-Sek J-Sek force-pushed the fix/20724 branch 2 times, most recently from 0769fea to f768a6b Compare April 2, 2025 21:36
@J-Sek
Copy link
Contributor Author

J-Sek commented Apr 2, 2025

Glad I had this test case for Portugal. It was incorrectly assigned during refactoring.

@johnleider
Copy link
Member

this PR can be merged instead of #20794 once we resolve changes to firstDay

What changes are left for firstDay?

@J-Sek
Copy link
Contributor Author

J-Sek commented Apr 3, 2025

What changes are left for firstDay?

None, all reverted. Hard to prove after refactoring into the concise switch though.. You'd need a script probably

const firstDay = {
  '001': 1,
  AD: 1,
  // etc.
}

console.log('codes for 0', Object.entries(firstDay).filter((_,v) => v === 0).map(([k,_]) => k).sort().join(' '))
console.log('codes for 1', Object.entries(firstDay).filter((_,v) => v === 1).map(([k,_]) => k).sort().join(' '))
console.log('codes for 6', Object.entries(firstDay).filter((_,v) => v === 6).map(([k,_]) => k).sort().join(' '))

johnleider
johnleider previously approved these changes Apr 3, 2025
@@ -37,6 +37,7 @@ export interface DateAdapter<T = unknown> {
getDiff (date: T, comparing: T | string, unit?: string): number
getWeekArray (date: T, firstDayOfWeek?: number | string): T[][]
getWeekdays (firstDayOfWeek?: number | string): string[]
getWeek (date: T, firstDayOfWeek?: number | string, firstWeekMinSize?: number): number
Copy link
Member

Choose a reason for hiding this comment

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

I know that I added new arguments to some of the typings but it was probably the wrong move. We need to figure out a way to get this information without needing to change the interface.

@KaelWD
Copy link
Member

KaelWD commented Apr 4, 2025

Sounds like we should just be using ISO rules for week of year https://unicode-org.atlassian.net/browse/CLDR-18275

@J-Sek
Copy link
Contributor Author

J-Sek commented Apr 4, 2025

Sounds like we should just be using ISO rules for week of year unicode-org.atlassian.net/browse/CLDR-18275

We could, but I can only see this being rolled out in Vuetify if we also let users opt-out with first-day-of-year prop or some other non-hacky way.

@J-Sek
Copy link
Contributor Author

J-Sek commented Apr 4, 2025

One last fix.. hopefully. I just realized when clicking on the playground - after choosing non-us locale, selecting startOfWeek: Sun was ignored.

@johnleider johnleider requested review from KaelWD and ikushum April 4, 2025 20:22
@johnleider johnleider merged commit 7321535 into vuetifyjs:master Apr 7, 2025
10 checks passed
@J-Sek J-Sek deleted the fix/20724 branch April 14, 2025 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VDatePicker E: date i18n Internationalization issue T: bug Functionality that does not work as intended/expected
Projects
None yet
4 participants