Datepicker enhancements #29

Blackbaud-SteveBrush · 2019-04-02T19:03:09Z

Removed requirement for template reference variable on wrapping datepicker component.
Reduced number of onChange calls when datepicker value is changed.
Created a datepicker test module.
Added visual test for invalid state.
Fixed calendar component to respect asynchronous selectedDate assignments.
Fixed datepicker AbstractControl to set appropriate values for dirty, touched, and invalid.
General code cleanup to respect code style guidelines.

codecov · 2019-04-02T19:41:28Z

Codecov Report

Merging #29 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #29   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         748    788   +40     
  Branches      132    143   +11     
=====================================
+ Hits          748    788   +40

Impacted Files	Coverage Δ
...ic/modules/datepicker/datepicker-config.service.ts	`100% <100%> (ø)`	⬆️
...c/modules/datepicker/datepicker-input.directive.ts	`100% <100%> (ø)`	⬆️
.../public/modules/datepicker/datepicker.component.ts	`100% <100%> (ø)`	⬆️
.../datepicker/datepicker-calendar-inner.component.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c5371e...ad81ab2. Read the comment docs.

Blackbaud-AlexKingman

Great work. Mostly, I have some questions around changing the public properties.

Blackbaud-AlexKingman · 2019-04-08T14:31:11Z

src/app/public/modules/datepicker/datepicker-calendar.component.spec.ts

@@ -419,6 +421,22 @@ describe('datepicker calendar', () => {

  });

+  it('should handle setting selected date asynchronously', fakeAsync(function () {


I might be missing something obvious, but how is this asynchronous? Also, you do very similar things in previous tests - I'm not sure what's different about this test...

It's a badly named unit test. I'll update the name so it makes more sense.

src/app/public/modules/datepicker/datepicker-input.directive.ts

Blackbaud-AlexKingman · 2019-04-08T15:18:39Z

src/app/public/modules/datepicker/datepicker.component.ts

-    this._startingDay = value;
-  }
+  @ViewChild(SkyDatepickerCalendarComponent)
+  private calendar: SkyDatepickerCalendarComponent;


This changed from public to private - was that intentional?

Yep! This is only useful to the internal workings of the datepicker component (and not being referenced anywhere else).

Blackbaud-AlexKingman · 2019-04-08T15:20:18Z

src/app/public/modules/datepicker/datepicker.component.ts


-  @ViewChild(SkyDropdownComponent)
-  public dropdown: SkyDropdownComponent;


Just checking - was removing this public property deliberate?

It was. It's not being accessed anywhere within the datetime library.

Blackbaud-AlexKingman · 2019-04-08T15:25:19Z

src/app/public/modules/datepicker/datepicker.component.ts


-  public dateSelected(newDate: Date) {


Removing a public property. Is this deliberate?

This was renamed to onSelectedDateChange to match our style guidelines (it was only being called from the datepicker's template).

Blackbaud-AlexKingman · 2019-04-08T15:25:33Z

src/app/public/modules/datepicker/datepicker.component.ts


-  public setSelectedDate(newDate: Date) {


Removing a public property. Is this deliberate?

I switched the public method to a setter field (public set selectedDate). It was only being used by the datepicker-input directive.

Blackbaud-AlexKingman · 2019-04-08T15:26:17Z

src/app/public/modules/datepicker/datepicker.component.ts


-  public setMinDate(_minDate: Date) {


Re: setMinDate / setMaxDate. I see you removed these references from the directive's OnChanges, but these are still public properties. Was removing these deliberate?

It was; we're not documenting these methods, and I would argue that if someone is using these directly, it's not the intent of the API.

Blackbaud-AlexKingman · 2019-04-08T15:45:50Z

Also, do we have an issue to update the docs/demo with the API changes?

blackbaud-johnly · 2019-04-08T15:59:06Z

I created blackbaud/skyux2-docs#399 to make sure we update the docs. ... Do we need another issue in the skyux2 repo for the demo?

Blackbaud-AlexKingman · 2019-04-08T17:12:34Z

@blackbaud-johnly thanks! Yes, I'd think so. We're deprecating a major part of how this API is used, so the demo should probably reflect that.

Blackbaud-TrevorBurch

Initial thoughts

src/app/public/modules/datepicker/datepicker-input.directive.ts

Blackbaud-TrevorBurch · 2019-04-08T15:56:12Z

src/app/public/modules/datepicker/datepicker-input.directive.ts

+  public ngOnChanges(changes: SimpleChanges): void {
+    if (changes.minDate) {
+      this.onValidatorChange();
+      this.datepickerComponent.minDate = this.minDate;


Is there a reason that we aren't just doing these things in the setters for the property? Seems like we could if we are going to the getters/setters for all of these properties.

src/app/public/modules/datepicker/datepicker-input.directive.ts

Blackbaud-TrevorBurch · 2019-04-08T17:09:00Z

src/app/public/modules/datepicker/datepicker.component.ts

-import { Subject } from 'rxjs/Subject';
-
-import { SkyDatepickerCalendarComponent } from './datepicker-calendar.component';
-
 import {


Shouldn't our libraries be below rxjs in the import ordering.

@skyux is on the same "level" as rxjs. I'm going with alphabetical order based on that.

Ok. I have always went with Angular -> Third party libraries -> Blackbaud libraries -> internal things. But I also see your logic :) Not a huge deal.

Blackbaud-TrevorBurch · 2019-04-08T17:19:35Z

src/app/visual/datepicker/datepicker-visual.component.ts

 } from '@angular/forms';

 @Component({
  selector: 'datepicker-visual',
  templateUrl: './datepicker-visual.component.html'
 })
 export class DatepickerVisualComponent implements OnInit {
-  public selectedDate: Date = new Date('4/4/2017');
+  public disabled = false;


Should we have a blank line before these public variables?

I could go either way. We can come up with a style guide if you like.

I personally like the line break and feel like it is more consistent with our other styles; however, I wouldn't consider it a blocker.

blackbaud-johnly · 2019-04-08T18:39:57Z

I created blackbaud/skyux2#2335 to update the demo.

Blackbaud-TrevorBurch

LGTM

Datepicker enhancements

8511059

Blackbaud-SteveBrush requested a review from Blackbaud-AlexKingman as a code owner April 2, 2019 19:03

This was referenced Apr 2, 2019

Reformat datepicker component #26

Closed

Date range module #30

Merged

Remove datepicker screenshots

c6f65b9

Blackbaud-SteveBrush added 5 commits April 2, 2019 17:13

Minor changes

e169d12

Revert date

db88215

Fix async calendar, visual tests

22ffb5d

Fix async calendar

7b06f49

Removed commented out code

b3267dd

Blackbaud-SteveBrush requested a review from Blackbaud-TrevorBurch April 3, 2019 14:41

Blackbaud-AlexKingman reviewed Apr 8, 2019

View reviewed changes

blackbaud-johnly mentioned this pull request Apr 8, 2019

Document datepicker enhancements blackbaud/skyux2-docs#399

Closed

Pull request feedback

304102c

Blackbaud-TrevorBurch suggested changes Apr 8, 2019

View reviewed changes

Addressed feedback

ad81ab2

blackbaud-johnly mentioned this pull request Apr 8, 2019

Update datepicker demo blackbaud/skyux2#2335

Closed

Blackbaud-TrevorBurch approved these changes Apr 8, 2019

View reviewed changes

Blackbaud-AlexKingman approved these changes Apr 8, 2019

View reviewed changes

Blackbaud-SteveBrush merged commit 405f125 into master Apr 8, 2019

Blackbaud-SteveBrush deleted the datepicker-enhancements branch April 8, 2019 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datepicker enhancements #29

Datepicker enhancements #29

Blackbaud-SteveBrush commented Apr 2, 2019 •

edited

Loading

codecov bot commented Apr 2, 2019 •

edited

Loading

Blackbaud-AlexKingman left a comment

Blackbaud-AlexKingman Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-AlexKingman Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-AlexKingman Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-AlexKingman Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-AlexKingman Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-AlexKingman Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-AlexKingman commented Apr 8, 2019 •

edited

Loading

blackbaud-johnly commented Apr 8, 2019

Blackbaud-AlexKingman commented Apr 8, 2019 •

edited

Loading

Blackbaud-TrevorBurch left a comment

Blackbaud-TrevorBurch Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-TrevorBurch Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-TrevorBurch Apr 8, 2019

Blackbaud-TrevorBurch Apr 8, 2019

Blackbaud-SteveBrush Apr 8, 2019

Blackbaud-TrevorBurch Apr 8, 2019

blackbaud-johnly commented Apr 8, 2019

Blackbaud-TrevorBurch left a comment

		@@ -419,6 +421,22 @@ describe('datepicker calendar', () => {

		});

		it('should handle setting selected date asynchronously', fakeAsync(function () {


		@ViewChild(SkyDropdownComponent)
		public dropdown: SkyDropdownComponent;

Datepicker enhancements #29

Datepicker enhancements #29

Conversation

Blackbaud-SteveBrush commented Apr 2, 2019 • edited Loading

codecov bot commented Apr 2, 2019 • edited Loading

Codecov Report

Blackbaud-AlexKingman 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

Blackbaud-AlexKingman commented Apr 8, 2019 • edited Loading

blackbaud-johnly commented Apr 8, 2019

Blackbaud-AlexKingman commented Apr 8, 2019 • edited Loading

Blackbaud-TrevorBurch 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

blackbaud-johnly commented Apr 8, 2019

Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

Blackbaud-SteveBrush commented Apr 2, 2019 •

edited

Loading

codecov bot commented Apr 2, 2019 •

edited

Loading

Blackbaud-AlexKingman commented Apr 8, 2019 •

edited

Loading

Blackbaud-AlexKingman commented Apr 8, 2019 •

edited

Loading