KDateComponents #384

LianaHarris360 · 2022-11-16T16:08:26Z

Description

This PR introduces the KDateRange modal component that contains two input components and a calendar component to select a date range. For translation flexibility, all strings used within the KDateRange can be provided as properties, and months are set using the dateLocale property.

Issue addressed

Closes #360

Before/after screenshots

View if there are no pre-selected start and end dates. The date input fields will be set to the placeholder.

View if there is a pre-selected start date given. The end date is set to the current day.

Steps to test

(optional) Implementation notes

The dateLocale property is a locale string used to set the language of the date strings and formatting. The month names shown on the calendar are automatically updated to reflect the language provided by this property.
The date objects are zero-indexed. January is month zero and December is month eleven. For example, if the firstAllowedDate is set to January 1, 2022, using JavaScript's date constructor, it should be created like this:
new Date(2022, 0, 1)

At a high level, how did you implement this?

The KDateRange uses the KModal component and also contains two KDateInput and KDateCalendar components that allow for a start and end date selection. The dates can be set by typing into the inputs or selecting a date from the calendar. The date inputs are validated with the use of a state machine that validates that the entered date is:

In the correct format
A valid date
After the first allowed date
Before the last allowed date
An end date that cannot be before the start date

On submission, the returned values are date objects.

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)?

Post-merge updates and tracking

Learning Platform update PR is submitted
Learning Platform update PR is merged
Studio update PR is submitted
Studio update PR is merged
Data Portal update PR is submitted
Data Portal update PR is merged

Comments

…ic added

commit f85ddfb Merge: 29122c4 9d29fe1 Author: akolson <[email protected]> Date: Thu Nov 3 22:42:28 2022 +0300 Merge pull request learningequality#380 from akolson/standardize-kradiobutton-labels Wraps KRadioButton label text commit 9d29fe1 Author: Samson Akol <[email protected]> Date: Wed Oct 26 23:34:47 2022 +0300 updates changelog commit ba2006c Author: Samson Akol <[email protected]> Date: Wed Oct 26 22:23:03 2022 +0300 updates documentation commit f083b25 Author: Samson Akol <[email protected]> Date: Wed Oct 26 22:04:02 2022 +0300 aligns kradiobutton api with kcheckbox's. Changes label from truncate to wrap. Adds truncateLabel prop to truncate text if necessary. updates documenation to reflect changes commit 29122c4 Merge: 115a19d 84ef450 Author: akolson <[email protected]> Date: Tue Oct 18 20:48:16 2022 +0300 Merge pull request learningequality#377 from akolson/implement-kResponsiveWindow-composable Implement k responsive window composable commit 84ef450 Author: Samson Akol <[email protected]> Date: Mon Oct 17 10:51:27 2022 +0300 Adds more review feedback commit 94abe9e Author: Samson Akol <[email protected]> Date: Fri Oct 14 19:16:35 2022 +0300 Improves test descriptions commit 6dc1d50 Author: Samson Akol <[email protected]> Date: Fri Oct 14 18:18:08 2022 +0300 Adds tests to useKWindowDimensions and useKResponsiveWindow commit 51d80a3 Author: Samson Akol <[email protected]> Date: Thu Oct 13 13:25:31 2022 +0300 minor string refactors commit 794ac2a Author: Samson Akol <[email protected]> Date: Thu Oct 13 12:55:00 2022 +0300 Fixes responsiveness bug on IE commit 445c740 Author: Samson Akol <[email protected]> Date: Wed Oct 12 23:01:34 2022 +0300 Adds feedback from review commit 68266f0 Author: Samson Akol <[email protected]> Date: Wed Oct 12 18:53:17 2022 +0300 Improves jsdoc, fixes IE issue commit b16c125 Author: Samson Akol <[email protected]> Date: Mon Oct 10 17:34:57 2022 +0300 Adds use composition-api to composable due to @vue/composition-api shortcomings in vue2 commit 4075c57 Author: Samson Akol <[email protected]> Date: Mon Oct 10 13:30:48 2022 +0300 Updates kresponsivewindow documentation commit 5940c76 Author: Samson Akol <[email protected]> Date: Mon Oct 10 12:38:45 2022 +0300 Adds feedback from code review commit b0fc205 Author: Samson Akol <[email protected]> Date: Thu Oct 6 18:27:24 2022 +0300 Updates changelog commit e504a70 Author: Samson Akol <[email protected]> Date: Thu Oct 6 15:52:29 2022 +0300 Code refactor commit 76c13a8 Author: Samson Akol <[email protected]> Date: Thu Oct 6 15:12:43 2022 +0300 Moves composition api initialization to useKResponsiveWindow commit 9dd5975 Author: Samson Akol <[email protected]> Date: Wed Oct 5 17:52:57 2022 +0300 Updates indentation commit 078c000 Author: Samson Akol <[email protected]> Date: Wed Oct 5 16:34:14 2022 +0300 uses anonymous function in documentation due to failures in character escaping commit ad681dd Author: Samson Akol <[email protected]> Date: Wed Oct 5 15:41:46 2022 +0300 Configures composition api, and documents the useKResponsiveWindow commit 2d1351e Author: Samson Akol <[email protected]> Date: Tue Oct 4 17:45:50 2022 +0300 Refactors code, adds documentation commit 8fe1519 Author: Samson Akol <[email protected]> Date: Mon Oct 3 18:59:22 2022 +0300 fixes bug in breakpoint media query listener commit f6afdf6 Author: Samson Akol <[email protected]> Date: Mon Oct 3 18:56:47 2022 +0300 Renames responsive window compodable to recommended composable file names commit c4278c2 Author: Samson Akol <[email protected]> Date: Fri Sep 30 23:15:15 2022 +0300 updates composable name commit fca8437 Author: Samson Akol <[email protected]> Date: Fri Sep 30 15:57:07 2022 +0300 Fixes bugs in composable commit e43b5a0 Author: Samson Akol <[email protected]> Date: Thu Sep 29 19:45:59 2022 +0300 Port of KResponsiveWindowMixin to KResponsiveWindow composable

…color constants

bjester

Left some code review comments. Overall, this looks really awesome!

An overview of my comments:

making sure all user facing strings will be translated
cross browser support and intricacies
ensuring HTML id's will be unique on a page

bjester · 2022-12-06T14:53:47Z

lib/KDateRange/KDateCalendar.vue

+        class="right"
+        size="mini"
+        @click="goNextMonth"
+      />


Looks like there are more user-facing strings in the button components above-- 'Previous Month' and 'Next Month'

Good catch, I missed these somehow! They should also be provided as properties for translation like the rest of the user-facing strings

bjester · 2022-12-06T14:59:09Z

lib/KDateRange/KDateCalendar.vue

+    },
+    watch: {
+      nextActiveMonth(value) {
+        if (value === 0) this.activeYearEnd = this.activeYearStart + 1;


I'm wondering if it's possible to be consistent and use another computed property for activeYear/nextActiveYear? I may not be understanding the reasons for this.

In this case it is because a computed value should not be mutated, referenced here:

a computed return value should be treated as read-only and never be mutated - instead, update the source state it depends on to trigger new computations

So when the Previous/Next Month buttons are clicked, the activeYearStart and activeYearEnd values will be updated directly and that will trigger the computed properties that depend on these values to update as well.

Yes, you shouldn't modify data in a computed prop, but that wasn't quite what I was referring to. So why did you choose a watcher instead of adding this code to the handlers for when the Previous/Next Month buttons are clicked? I do see code in those handlers that are modifying activeYearEnd, so it isn't clear why both are needed?

With regards to this specific code-- what happens if the picker is on the month of December, and I click Next, then Previous, then Next, then Previous, and so on? Does this.activeYearEnd just keep on incrementing? I suspect the code in the button handlers is possibly covering up a potential issue?

Okay, I misunderstood your question at first. The watcher was chosen as a convenient way of keeping tabs on when the nextActiveMonth becomes 0 (January) because that is when activeYearEnd should be updated to the next year.

When goNextMonth is pressed, the activeMonth is what is actually being updated, not activeYearEnd, so you are right, changing activeYearEnd inside of the goNextMonth method is redundant.

activeYearEnd only needs to update when nextActiveMonth becomes 0 or when goPrevMonth is pressed and activeYearStart isn't the same value as activeYearEnd, which indicates that the right half of the calendar is in a different year. So pressing goPrevMonth would put both sides of the calendar in the same year when it updates the month, this is what decrements the activeYearEnd.

I thought the watcher was appropriate because nextActiveMonth is only changed after activeMonth is updated (which happens inside goNextMonth), so the watcher allows activeYearEnd to be updated only when the computed property nextActiveMonth is a certain number.

But, based on what you're suggesting, I should be able to make activeYearEnd and activeYearStart computed properties where the original values are set using activeMonth, so they should both update if activeMonth is updated and I can still check the value of nextActiveMonth when updating activeYearEnd. I can implement it this way and update you if I have any issues!

bjester · 2022-12-06T15:01:26Z

lib/KDateRange/KDateCalendar.vue

+        if (!this.isFirstChoice && resultDate < this.dateRange.start) {
+          this.isFirstChoice = false;
+          return { start: resultDate };
+        }


Since this if block returns early, and doesn't depend on the code above (that I can see), would be good to move it to the beginning of the function.

bjester · 2022-12-06T15:01:53Z

lib/KDateRange/KDateCalendar.vue

+        if (!this.isFirstChoice) {
+          key = 'end';
+        } else {
+          newData['end'] = null;


newData.end is okay here

bjester · 2022-12-06T15:06:22Z

lib/KDateRange/KDateCalendar.vue

+          lastDay = new Date(this.activeYearStart, this.activeMonth + 1, 0).getDate();
+        } else {
+          lastDay = new Date(this.activeYearEnd, this.nextActiveMonth + 1, 0).getDate();
+        }


Can be a ternary statement:

const lastDay = (key === 'first') ? new Date(this.activeYearStart, this.activeMonth + 1, 0).getDate() : new Date(this.activeYearEnd, this.nextActiveMonth + 1, 0).getDate();

bjester · 2022-12-06T15:07:07Z

lib/KDateRange/KDateCalendar.vue

+        this.activeYearEnd = nextMonth.getFullYear();
+      },
+      isValidDate(date) {
+        return new Date(date) !== 'Invalid Date' && !isNaN(new Date(date));


Is 'Invalid Date' browser specific? Also, what if they have their browser in a different language?

This is a good point, maybe the !isNaN(new Date(date)) should be enough to validate the date argument.

bjester · 2022-12-06T15:09:52Z

lib/KDateRange/KDateCalendar.vue

+  }
+
+  .calendar-days li.calendar-days--disabled {
+    color: var(--disabled-button-color);


Do we use CSS vars elsewhere in KDS? Seems there isn't support for them in IE 11

I don't think we use them. That said, working with the theme tokens, plus active and disabled styles, can be a bit convoluted with our theming system and isn't always intuitive.

The KRadioButton code is an example of using a combination of computed styles and :style within the component to access the token values. @LianaHarris360 let me know if you have questions after reviewing this, or want to work together on how you could use this pattern in your component.

Just to affirm here that we can't use CSS vars in KDS, because of the lack of IE11 support. We've investigated a ponyfill, but not implemented it yet, due to the technical complexity (and because it is not a complete polyfill it comes with a bunch of caveats).

In general, using computed :style attributes is preferable, as it is a little simpler to reason about, but (especially for pseudo-selectors, which cannot be applied via style attributes) the $computedClass helper function lets you generate arbitrary CSS in JS: https://design-system.learningequality.org/colors/#usage

Thank you @marcellamaki and @rtibbles!

That definitely makes sense, the browser support is an important factor that I missed! Thank you for the references, I'm going to reimplement the styling with these suggestions and will reach out if I need more support on this.

bjester · 2022-12-06T15:13:16Z

lib/KDateRange/KDateInput.vue

+      />  
+      <input type="hidden" name="date" :value="valueAsDate" data-test="valueAsDate">
+      <span class="k-date-vhidden">
+        <span v-if="valueAsDate" id="date-desc">


id="date-desc" 👀 -- What happens if there are multiple KDateInput components on the same page?

Good point, I remember now that we did talk about a solution that generates unique ids. I will go back and add that.

bjester · 2022-12-06T15:14:03Z

lib/KDateRange/KDateInput.vue

+          isNaN(this.valueAsDate)
+        ) {
+          return ' ';
+        } else {


Since the prior if block returns, you can safely extract this from the else, like you did with valueAsDate. Makes the code a little cleaner

bjester · 2022-12-06T15:16:03Z

lib/KDateRange/ValidationMachine.js

+  return isAfter(newDate, lastAllowedDate);
+};
+
+/** Returns if the given date string is before the first allowed date */


For these functions above, any reason not to also document the arguments, like you've done for validate?

There is no reason not to. I can see how documenting the arguments makes things clearer, so I'll update these functions.

LianaHarris360 · 2022-12-06T15:45:56Z

Left some code review comments. Overall, this looks really awesome!

An overview of my comments:

making sure all user facing strings will be translated

cross browser support and intricacies

ensuring HTML id's will be unique on a page

Thank you for the review, I will work on clarifying the code and making updates with these suggestions!

marcellamaki

Hi @LianaHarris360 - I've added just a couple of comments here to follow up on some of what @bjester said regarding css variables. Let me know if you have any questions!

marcellamaki · 2022-12-06T18:45:40Z

lib/KDateRange/KDateCalendar.vue

+  }
+
+  .calendar-days li.calendar-days--disabled {
+    color: var(--disabled-button-color);


I don't think we use them. That said, working with the theme tokens, plus active and disabled styles, can be a bit convoluted with our theming system and isn't always intuitive.

The KRadioButton code is an example of using a combination of computed styles and :style within the component to access the token values. @LianaHarris360 let me know if you have questions after reviewing this, or want to work together on how you could use this pattern in your component.

marcellamaki · 2022-12-06T18:48:32Z

lib/KDateRange/KDateDay.vue

+          '--selected-button-text-color': this.$themePalette.white,
+          '--in-range-button-bg-color': this.$themeBrand.secondary.v_50,
+          '--in-range-button-text-color': this.$themePalette.grey.v_700,
+          '--in-range-button-hover-color': this.$themePalette.grey.v_200,


Similar to above for here. I actually think it's okay to use a computed prop to come up with your button styles, but I think using :style or :class within the component itself would be more consistent with our current patterns (and, as @bjester mentioned, prevent potential problems with IE11). Again, happy to help brainstorm how to do this -- I know there are a lot of different conditions that are being managed with the button and date range logic!

marcellamaki · 2022-12-06T18:50:36Z

lib/KDateRange/KDateInput.vue

+<template>
+
+  <div>
+    <fieldset :aria-labelledby="legendText" class="date-input-fieldset" aria-describedby="date-desc" aria-live="polite">


Love to see accessibility in the code from the very first PRs! Awesome!! 🎉

Taking another look here, I believe aria-labelledby is supposed to accept element ID list. Is this passing a text string?

The aria-labelledby property takes as value a space-separated id reference list

Yes this is correct, in this case, the aria-labelledby should be aria-label or should maybe point to the KTextBox element. There was previously a legend and the aria-labelledby was set to that, but when testing with the chrome extension screen reader, the label would be announced twice so I removed the legend and did not update aria-labelledby accordingly. Thank you for pointing this out!

LianaHarris360 · 2022-12-06T22:22:37Z

Hi @LianaHarris360 - I've added just a couple of comments here to follow up on some of what @bjester said regarding css variables. Let me know if you have any questions!

@marcellamaki - I will let you know if I have any questions for sure. I am currently looking into those potential solutions, thank you!

…ngs, removed css variable usage, and updated calendar computed properties

marcellamaki

This looks great, @LianaHarris360 - thank you so much for your hard work on this! I think all of the PR feedback has been addressed now, and I feel good about merging this into KDS develop and then you moving forward with your work using it within Kolibri. There's always a chance an edge case or bug might pop up, but that's completely fine. I think we're most likely to surface that as you and the rest of the team begin to use it, and we can continue to build on the component as needed.

🎉 Amazing, amazing work! 🎉

LianaHarris360 added 9 commits October 23, 2022 01:24

K-date-range components added

83a2c67

KModal, date inputs, calendar, and KButton styling and validation log…

3a89acb

…ic added

Error handling, corrected imports, and code clean-up changes

f3e810d

Linting and auto formatter changes

f02c2f8

Validation state machine added

ac467e5

KDateInput validation refactored and validation machine updated

bdc6b16

First allowed date correction

2673062

Submit disabled validation corrected

88931e2

LianaHarris360 requested a review from bjester November 16, 2022 16:08

LianaHarris360 added 6 commits November 17, 2022 09:51

Validation machine tests completed and calendar styling updated with …

99e2034

…color constants

Tests for KDateRange and KDateInput added

1fc73e0

Test added

c641566

Date locale property added to components

88133a5

Updated changelog and fixed some formatting

bb73e7b

Merge branch 'develop' into k-date-components

f3d3637

LianaHarris360 marked this pull request as ready for review December 1, 2022 17:16

bjester reviewed Dec 6, 2022

View reviewed changes

LianaHarris360 requested a review from marcellamaki December 6, 2022 16:49

marcellamaki reviewed Dec 6, 2022

View reviewed changes

LianaHarris360 added 2 commits December 8, 2022 13:33

Implemented code review suggestions, added props for user-facing stri…

7ed8ba8

…ngs, removed css variable usage, and updated calendar computed properties

Merge branch 'develop' into k-date-components

9fe98f6

LianaHarris360 requested review from bjester, rtibbles and marcellamaki December 8, 2022 22:26

marcellamaki approved these changes Jan 5, 2023

View reviewed changes

marcellamaki merged commit d4226b5 into learningequality:develop Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KDateComponents #384

KDateComponents #384

LianaHarris360 commented Nov 16, 2022 •

edited

Loading

bjester left a comment

bjester Dec 6, 2022

LianaHarris360 Dec 7, 2022

bjester Dec 6, 2022

LianaHarris360 Dec 7, 2022

bjester Dec 7, 2022

LianaHarris360 Dec 7, 2022

bjester Dec 6, 2022

bjester Dec 6, 2022

bjester Dec 6, 2022

bjester Dec 6, 2022

LianaHarris360 Dec 7, 2022

bjester Dec 6, 2022

marcellamaki Dec 6, 2022

rtibbles Dec 6, 2022

bjester Dec 6, 2022

LianaHarris360 Dec 7, 2022

bjester Dec 6, 2022

LianaHarris360 Dec 7, 2022

bjester Dec 6, 2022

bjester Dec 6, 2022

LianaHarris360 Dec 7, 2022

LianaHarris360 commented Dec 6, 2022

marcellamaki left a comment

marcellamaki Dec 6, 2022

marcellamaki Dec 6, 2022

marcellamaki Dec 6, 2022

bjester Dec 7, 2022

LianaHarris360 Dec 7, 2022 •

edited

Loading

LianaHarris360 commented Dec 6, 2022

marcellamaki left a comment

KDateComponents #384

KDateComponents #384

Conversation

LianaHarris360 commented Nov 16, 2022 • edited Loading

Description

Issue addressed

Before/after screenshots

Steps to test

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Reviewer guidance

Post-merge updates and tracking

Comments

bjester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LianaHarris360 commented Dec 6, 2022

marcellamaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LianaHarris360 Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

LianaHarris360 commented Dec 6, 2022

marcellamaki left a comment

Choose a reason for hiding this comment

LianaHarris360 commented Nov 16, 2022 •

edited

Loading

LianaHarris360 Dec 7, 2022 •

edited

Loading