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

Corrected kdaterange month and year display and removed font-family #406

Merged

Conversation

LianaHarris360
Copy link
Member

@LianaHarris360 LianaHarris360 commented Jan 23, 2023

Description

This PR fixes the incorrect display of the Month Year on the KDateRange calendar when the previous month is December of last year and the current month is January. Previously, the left side of the calendar was incorrectly set to December of this year and the right side of the calendar was set to January of next year.

The font-family declaration has also been removed from KDateCalendar as it was unnecessary and caused implementation issues when KDateRange is used in Kolibri.

Added predictableActionArguments: true to xstate validation machine to fix console warning based on XState Docs recommendations found here.

Issue addressed

Fixes #405

Before/after screenshots

Before, incorrect Month Year display
CorrectKDateRange

After, correct Month Year display
IncorrectKDateRange

Steps to test

  1. Open KDateRange calendar modal demo.
  2. Confirm that left side of calendar displays December of last year and the right side of the calendar displays January of this year.

(optional) Implementation notes

At a high level, how did you implement this?

The activeMonth (the month that is displayed on the left side of the calendar) is set with new Date().getMonth() - 1.

When new Date().getMonth() returns 0 for January, the value returned from new Date().getMonth() - 1 is -1 however, it should be 11, for December. Now if new Date().getMonth() - 1 returns -1, activeMonth is instead set to 11.

In created(), if activeMonth is 11, activeYearStart is set to last year.

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change has been added to the changelog

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

I'm not sure if this change needs to be added in the changelog or not.

@LianaHarris360 LianaHarris360 marked this pull request as ready for review January 23, 2023 17:15
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Thanks @LianaHarris360. The fix LGTM!

@@ -98,6 +98,7 @@ export const initialContext = {
};

export const validationMachine = createMachine({
predictableActionArguments: true,
Copy link
Member

Choose a reason for hiding this comment

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

Ooh - I wonder if I should include this in the SetupWizard machine too!

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

XState warning
3 participants