feat(datepicker): add support for date range and different date formats #1381

venkateshr06 · 2024-05-02T06:58:01Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Issue Number: CDE-1846

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

github-actions · 2024-05-02T06:58:20Z

👋 @venkateshr06,

🙏 The Clarity team thanks you for opening a pull request
🎉 The build for this PR has succeeded
🔍 The PR is now ready for review
🍿 In the meantime, view a preview of this PR
🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal Clarity Support space

Thank you,

🤖 Clarity Release Bot

github-actions · 2024-05-06T04:34:48Z

This PR introduces visual changes: d57a180
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout main
git fetch https://github.com/vmware-clarity/ng-clarity.git d57a18077e0687e10b3f70205e5ef8e12e4a4276
git cherry-pick d57a18077e0687e10b3f70205e5ef8e12e4a4276
git push

github-actions · 2024-05-06T10:40:10Z

This PR introduces visual changes: 3450769
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout main
git fetch https://github.com/vmware-clarity/ng-clarity.git 34507699f33544178327dd1b5b734c6b7e9dac25
git cherry-pick 34507699f33544178327dd1b5b734c6b7e9dac25
git push

github-actions · 2024-05-07T04:52:31Z

This PR introduces visual changes: 400af37
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout main
git fetch https://github.com/vmware-clarity/ng-clarity.git 400af3754208100aba0af43019a868580055606e
git cherry-pick 400af3754208100aba0af43019a868580055606e
git push

github-actions · 2024-05-07T09:56:27Z

This PR introduces visual changes: 9b748ad
If these changes are intended and correct, please cherry-pick the above commit to this PR.

git checkout main
git fetch https://github.com/vmware-clarity/ng-clarity.git 9b748adec6d3e0bbae21cef7bc651750e87239e1
git cherry-pick 9b748adec6d3e0bbae21cef7bc651750e87239e1
git push

Jinnie

First, thanks for the great amount of work on this exciting feature and all the attention to fine details and various use-cases!
Reviewing this may take some more iterations, but right now I have some high level questions and concerns that we may further discuss offline.

My goals, as reviewer, are for now the following:

Ensure we're not breaking current behavior/s and provide incremental API changes where possible.
Make sure we are providing a solid and well structured architecture of the various sub-components.
Validate agains the design requirements and guidelines.

First off, my biggest concern is that the implementation diverges too much from the initial specs that were presented to the team. This leads to unexpected scope explosion and features and architecture that were not synced on planning phase.

The changes on the abstract and other common classes bring concerns (explained in more detail in the lower comments). They bring (seemingly) unnecessary specificity to a high hierarchy level, and drag along changes in event model, subscriptions, validation and more.

The API and naming of the existing date picker has been change witch causes breaking changes, which we would prefer to avoid.

Some of the use-cases are not realistic - like the month range (without year). For example - if you select Feb to Sept it may make some sense, but what happens if you select Sept to Feb?

A large volume of commented code is left. I added comments to most, but not all cases, please check. Especially in tests, where it's not clear if these tests are obsolete, or just temporarily commented and still needs to get satisfied.

projects/angular/src/forms/datepicker/date-single-input.ts

projects/angular/src/forms/common/abstract-container.ts

projects/angular/src/forms/datepicker/_datepicker.clarity.scss

projects/angular/src/forms/datepicker/date-input-base.ts

projects/angular/src/forms/datepicker/model/calendar-view.model.ts

projects/angular/src/forms/datepicker/providers/date-io.service.ts

Jinnie

It would really be nice to have the dateFormat related work in a separate PR.
As it fights with default browser localization and provider-based localization we don't even know if we want it, or when we want it released and now it's tied to the range component.

And some more observations and first use impressions:

Dragging a range, as opposed to sequential clicking, does not work. Not sure this is valid use case, but I hit it several times before understanding what happens. May need to sync with UX.
Trying to only change the end date on already existing value resets the start date. UX comment needed.
Sequentially clicking backwards, from later to earlier date, just resets the start date. UX comment needed.
The dash (-) separating start and end date is too much closer to end date making it look like negative value instead of range. I know the technical reasons for this, but some reiteration with Design might help here.
Nit: after selecting a range option, the next time you open the picker it shows "Custom range". Not critical, but would be nice if we could resolve back the range option and pre-select it if it's matching.
Nit: If we violate validation restrictions in both fields we're receiving just one error message.

projects/demo/src/app/datepicker/datepicker-DE.ts

projects/demo/src/app/datepicker/datepicker-in-reactive-forms.html

projects/demo/src/app/datepicker/datepicker-date-input-explicit-wrapper.html

projects/demo/src/app/datepicker/datepicker-in-reactive-forms.ts

Jinnie · 2024-05-14T10:26:48Z

projects/demo/src/app/datepicker/datepicker.demo.html

+    </li>
+    <li>
+      <a [routerLink]="['./enhanced-date-picker']"
+        >Enhanced Date Picker


Maybe we should name it "Custom Date Format" or something in that direction. "Enhanced" is too broad.

projects/demo/src/app/datepicker/datepicker.demo.scss

.storybook/stories/datepicker/daterangepicker.stories.ts

kevinbuhmann · 2024-06-12T14:55:58Z

It would really be nice to have the dateFormat related work in a separate PR.
As it fights with default browser localization and provider-based localization we don't even know if we want it, or when we want it released and now it's tied to the range component.

I agree. These two features need to be separated. There is too much in this PR. These things should be reviewed and merged separately. And we might release one sooner than the other. Neither feature should held up by issues with the other.

projects/angular/src/forms/common/if-control-state/if-control-state.service.ts

Jinnie · 2024-06-13T10:23:28Z

projects/angular/src/forms/common/if-control-state/if-control-state.service.ts

+          // and emit the control
+          this.additionalControls?.forEach((control: NgControl) => {
+            this.subscriptions.push(
+              control?.statusChanges?.subscribe(() => {


If additional controls get changed, won't we need to unsubscribe from the previous statusChanges subscriptions before we register the new ones?
This may apply for the primary control subscriptions too, if my previous comment gets resolved.

Set control is going to happen only once per control if I am not wrong. I doubt that we can change the control i mean replace the existing control with new one. It will happen when container itself is completely destroyed and recreated. Also this is an existing implementation which can be found in other file as well(select-container).

Do you think it might become more explicit if we add take(1)?
We won't even need to add it to the subscriptions array if we do. We're using the take(1) approach in several components and services already.

Totally agree, but since its an existing approach in all other files didn't change the approach. Need to do some research why it was added in this way. Hence keeping it in subscriptions mode as of now.

My concern is that we're adding more subscriptions inside a subscribe block. This may lead to leaks. It should be okay to add take(1) here. It will make the code safer if it really only gets called once, or will throw explicit error if it unexpectedly gets called more than once, and save us tedious performance/memory-leak debugging.

projects/angular/src/forms/common/if-control-state/if-error.ts

Jinnie · 2024-06-13T10:46:13Z

projects/angular/src/forms/common/wrapped-control.ts

+    this.triggerDoCheck(this.differ, this.ngControl);
+  }
+
+  triggerDoCheck(differ, ngControl) {


Why do we need this as separate public method? I don't see it used anywhere else.

Basically added it as a separate methods in order to support Multiple Form controls(Primary & Additional Controls). Removed Purposefully Keyvalue differ for additional Controls now its added back.

It's only called from the ngDoCheck method above. If you want to keep it for better code structure and readability, I am okay with this, but it can be private.

projects/angular/src/forms/common/wrapped-control.ts

Jinnie · 2024-06-13T11:03:37Z

projects/angular/src/forms/datepicker/date-input-base.ts

+   * Incase of date range error, both start & end date field valdiation has to be triggered
+   * if either of the field gets updated
+   */
+  protected validateDateRange() {


I believe this belongs to the range specific children. They may have another common parent, that extends this class, to avoid code duplication between start and end, though.

Yes, since both start date and end date component are using this kept in date-input-base component. Anyways I have wrapped it inside condition to check if its date range picker.

Jinnie

A lot of code is duplicated between the date-range-start/end-input directives.

I would also like to have some UX input whether next/prev month grayed-out dates should be excluded from the range visualization, if it extends between adjacent months:

vmwclabot added the cla-not-required label May 2, 2024

venkateshr06 closed this May 2, 2024

venkateshr06 reopened this May 2, 2024

kevinbuhmann changed the title ~~feat(datepicker): Add Support for Date Range & different date formats~~ feat(datepicker): add support for date range and different date formats May 2, 2024

venkateshr06 self-assigned this May 6, 2024

venkateshr06 marked this pull request as ready for review May 6, 2024 08:15

venkateshr06 requested a review from a team May 9, 2024 04:33

Jinnie reviewed May 13, 2024

View reviewed changes

Jinnie reviewed May 14, 2024

View reviewed changes

Jinnie reviewed Jun 12, 2024

View reviewed changes

.storybook/stories/datepicker/daterangepicker.stories.ts Outdated Show resolved Hide resolved

Jinnie reviewed Jun 13, 2024

View reviewed changes

projects/angular/src/forms/common/if-control-state/if-control-state.service.ts Outdated Show resolved Hide resolved

Jinnie reviewed Jun 13, 2024

View reviewed changes

projects/angular/src/forms/common/if-control-state/if-error.ts Outdated Show resolved Hide resolved

Jinnie reviewed Jun 13, 2024

View reviewed changes

projects/angular/src/forms/common/wrapped-control.ts Outdated Show resolved Hide resolved

Jinnie reviewed Jun 13, 2024

View reviewed changes

github-actions bot added the flag:stale label Sep 16, 2024

venkateshr06 closed this Nov 5, 2024

venkateshr06 force-pushed the main branch from cb8708e to d1ae4a9 Compare November 5, 2024 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): add support for date range and different date formats #1381

feat(datepicker): add support for date range and different date formats #1381

venkateshr06 commented May 2, 2024 •

edited

Loading

github-actions bot commented May 2, 2024 •

edited

Loading

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 7, 2024

github-actions bot commented May 7, 2024

Jinnie left a comment •

edited

Loading

Jinnie left a comment

Jinnie May 14, 2024

kevinbuhmann commented Jun 12, 2024 •

edited

Loading

Jinnie Jun 13, 2024

venkateshr06 Jun 14, 2024

Jinnie Jun 14, 2024

venkateshr06 Jun 14, 2024

Jinnie Jun 17, 2024

Jinnie Jun 13, 2024

venkateshr06 Jun 14, 2024

Jinnie Jun 17, 2024

Jinnie Jun 13, 2024

venkateshr06 Jun 14, 2024

Jinnie left a comment

feat(datepicker): add support for date range and different date formats #1381

feat(datepicker): add support for date range and different date formats #1381

Conversation

venkateshr06 commented May 2, 2024 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

github-actions bot commented May 2, 2024 • edited Loading

github-actions bot commented May 6, 2024

github-actions bot commented May 6, 2024

github-actions bot commented May 7, 2024

github-actions bot commented May 7, 2024

Jinnie left a comment • edited Loading

Choose a reason for hiding this comment

Jinnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbuhmann commented Jun 12, 2024 • edited Loading

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

Jinnie left a comment

Choose a reason for hiding this comment

venkateshr06 commented May 2, 2024 •

edited

Loading

github-actions bot commented May 2, 2024 •

edited

Loading

Jinnie left a comment •

edited

Loading

kevinbuhmann commented Jun 12, 2024 •

edited

Loading