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(VDatePicker): weeknumber calculation refactored to fix bug #10926

Merged
merged 10 commits into from
Apr 20, 2020

Conversation

abbierwolf
Copy link
Contributor

Instead of calculating only the weeknumber for the first week shown and increment it, it is now
calcuted for every week.

fix #9764

Description

fixes #9764

Motivation and Context

#9764

How Has This Been Tested?

unit | visually

Markup:

<template>
  <v-container>
    <!--  -->
    <v-date-picker
      show-week
      first-day-of-week="1"
    ></v-date-picker>
  </v-container>
</template>

<script>
export default {
  data: () => ({
    //
  })
};
</script>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and backwards compatible changes and next for non-backwards compatible changes).
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

@johnleider johnleider changed the title fix(vdatepicker): weeknumber calculation refactored to fix bug fix(VDatePicker): weeknumber calculation refactored to fix bug Mar 24, 2020
@johnleider johnleider added this to the v2.2.x milestone Mar 24, 2020
@johnleider johnleider added C: VDatePicker VDatePicker T: bug Functionality that does not work as intended/expected labels Mar 24, 2020
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.

Need to clean up the code styling. I'm surprised it passed lint. More information is located in our Coding Guidelines.

@abbierwolf
Copy link
Contributor Author

abbierwolf commented Mar 24, 2020

I applied all coding guidelines I could find in the document. I could not find tslint or eslint configuration to make sure I found all the issues.

@abbierwolf
Copy link
Contributor Author

@johnleider Travis CI had problems yesterday can you trigger a rebuild? Or is this not needed?

Copy link
Member

@jacekkarczmarczyk jacekkarczmarczyk left a comment

Choose a reason for hiding this comment

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

Could it be done without using the Date object? I remember it's having some problems on ios/osx and with different time zones.

Some magic numbers (like 604800000) could be turned into a constant with a meaningful name and written as a product of some better recognized values (for example 86400 = 24(hrs) * 60(min) * 60(src))

Last but not least method could be extracted to helpers so it could be tested wihtout involving all the component

@johnleider
Copy link
Member

@jacekkarczmarczyk Please leave that in a review.

packages/vuetify/src/components/VDatePicker/VDatePicker.ts Outdated Show resolved Hide resolved
Number(this.firstDayOfWeek)
) % 7 // https://en.wikipedia.org/wiki/Zeller%27s_congruence
return Math.floor((dayOfYear + offset) / 7) + 1
getWeekNumber (dayInMonth: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Re-format this similar to:

return weekNumber(
  arg1,
  arg2,
  arg3,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First argument is on its own line

Copy link
Member

Choose a reason for hiding this comment

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

This was meant for line 69

packages/vuetify/src/util/__tests__/dateTimeUtils.spec.ts Outdated Show resolved Hide resolved
packages/vuetify/src/util/dateTimeUtils.ts Outdated Show resolved Hide resolved
@johnleider johnleider merged commit b7f830a into vuetifyjs:master Apr 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: VDatePicker VDatePicker T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] v-date-picker displays the wrong week number when first-day-of-week="1"
4 participants