Time Value Component #36

mdelez · 2020-02-26T14:54:30Z

closes #16

…me value

…er a correct date with the picker

mdelez · 2020-03-02T14:22:25Z

The tests aren't working very well since my local tests rely on my local time zone and the CI operates on a different time zone so the values I check for are always wrong on one of the tests (either local tests or the CI test)

mdelez · 2020-03-03T09:14:20Z

I think using this library would be ideal since it combines a date picker and a time picker. Unfortunately it requires Angular 9. There is an old version for Angular 8 but it is no longer supported.

mdelez · 2020-03-04T12:09:16Z

Running the code on localhost works with no issue and returns the correct timestamp. The offset for us from UTC is +2 hours so an offset value of -120 is correct (value is in minutes and a positive offset returns a negative number).

Running the unit test fails because for some reason the offset is -34. I have no idea where this seemingly random offset is coming from. From my understanding of time (which is a bit unclear now), except for a couple of timezones, namely some Australian timezones, every offset only deals with hours, not minutes or seconds. This adds to my confusion because in the tests I am setting the time value to "22:00" and it is returning a timestamp of "1776-07-04T21:25:52.000Z" when I should be expecting "1776-07-04T20:00:00.000Z" to account for the offset. Somehow the offset is also affecting the minutes and seconds.

Update
I've figured out the issue...kinda. I was using a rather old date for testing and this is what was causing me to receive an offset of -34. I changed the value to a more recent value (2019) and the tests run successfully now. Knora handles the offset perfectly and always returns to correct time so no issues there.

I've opened up a new issue for this #38

… changed the comment input to a text area

flavens · 2020-03-06T08:58:30Z

projects/knora-ui/src/lib/viewer/operations/display-edit/display-edit.component.scss

@@ -0,0 +1,3 @@
+::ng-deep .parent-value-component .mat-form-field {


for your information: https://angular.io/guide/component-styles#deprecated-deep--and-ng-deep

😣technology evolves too fast. I'll update my code. Thanks!

::ng-deep, /deep/ and >>> deprecation
The ::ng-deep pseudo-class selector also has a couple of aliases: >>> and /deep/, and all three are soon to be removed.

The main reason for that is that this mechanism for piercing the style isolation sandbox around a component can potentially encourage bad styling practices.

The situation is still evolving, but right now, ::ng-deep can be used if needed for certain use cases.

https://blog.angular-university.io/angular-host-context/

From what I can find online, Angular has marked the ::ng-deep selector as deprecated but has not yet provided a new solution. They recommend to use ::ng-deep only if absolutely necessary so I shall use it sparingly.

…o remove as much deprecated styling as possible

tobiasschweizer · 2020-03-11T08:42:04Z

@mdelez Thanks, I will look at this PR today.

…mponent to the child component and cleaned up a lot of repeated code

…hey can be more easily tested

mdelez · 2020-03-12T10:50:43Z

I've refactored a lot my code and I think it looks quite clean now. Namely I have moved all of the date conversion and logic to the child component and now the parent component is only concerned about a datetime string. I've also moved the date conversion logic into separate methods so that it is easier to test.

Some minor things still left to do:

move html and css changes to separate PR
work on the time validator so that it operates correctly within Safari since Safari doesn't support the "time" type within an input

tobiasschweizer

If I hit the cancel button for the time part

I get:

TimeInputComponent.html:8 ERROR RangeError: Invalid time value
at Date.toISOString ()
at TimeInputComponent.userInputToTimestamp (knora-ui.js:1822)
at TimeInputComponent.get value [as value] (knora-ui.js:1736)
at TimeInputComponent._handleInput (knora-ui.js:1810)
at Object.eval [as handleEvent] (TimeInputComponent.html:8)
at handleEvent (core.js:43993)
at callWithDebugContext (core.js:45632)
at Object.debugHandleEvent [as handleEvent] (core.js:45247)
at dispatchEvent (core.js:29804)
at core.js:42925

Probably this is because a cancelled time is null.

tobiasschweizer · 2020-03-12T10:57:59Z

projects/knora-ui/src/lib/viewer/values/time-value/time-value.component.ts

+    return (control: AbstractControl): { [key: string]: any } | null => {
+
+      const invalid = initValue === control.value &&
+                      control.value.split(':').pop().split(';')[0].match(CustomRegex.TIME_REGEX) == null &&


I think yo don't need to validate the time in the parent since you are doing that already in the child component.

In the best case overriding the standardValidatorFunc becomes unnecessary.

fixed error and removed overridden validator in bfec41b

tobiasschweizer · 2020-03-12T10:59:22Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+    this.dateFormControl = new FormControl(null);
+    this.dateFormControl.setValidators([Validators.required]);
+
+    this.timeFormControl = new FormControl({value: null, Validators: [Validators.required, this.timeValidator]});


I think you can directly put Validators.pattern(CustomRegex.TIME_REGEX) here.

fixed in bfec41b

tobiasschweizer · 2020-03-12T11:04:11Z

projects/knora-ui/src/lib/viewer/values/time-value/time-value.component.html

+      <mat-error *ngIf="form.controls.timeValue.hasError('valueNotChanged')">
+        Please change the value
+      </mat-error>
+    </mat-form-field>


Could you add some space or a line break between the value and comment?

in Chrome:

It looks okay on my screen but I know the css needs some improvement. I'll move the css to a new PR and add a line break as the last thing before merging.

tobiasschweizer · 2020-03-12T11:13:13Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+    super(_defaultErrorStateMatcher, _parentForm, _parentFormGroup, ngControl);
+
+    this.dateFormControl = new FormControl(null);
+    this.dateFormControl.setValidators([Validators.required]);


I think you can use the more compact form like below to set the validator.

fixed in bfec41b

tobiasschweizer · 2020-03-12T11:20:56Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+  }
+
+  // converts and returns a unix timestamp string as an array consisting of a GregorianCalendarDate and a string
+  convertTimestampToDateTime(timestamp: string): { gcd: GregorianCalendarDate, time: string }[] {


Could you make the return type an object with one member being time and the other the date?

What is { gcd: GregorianCalendarDate, time: string }[] supposed to mean? An array of objects each having a member gcd and time?

Yeah, returning an object would probably be better. Currently it's returning an array of objects because gcd and time are different types

fixed in bfec41b

tobiasschweizer · 2020-03-12T11:22:05Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+  }
+
+  // return converted Date obj as a string without the milliseconds
+  userInputToTimestamp(userInput: any): string {


Could you give the argument userInputa type annotation other than any?

Probably you can make it consistent with what you return from convertTimestampToDateTime.

fixed in 6392c37

tobiasschweizer · 2020-03-12T11:32:20Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+
+  // return converted Date obj as a string without the milliseconds
+  userInputToTimestamp(userInput: any): string {
+    let splitTime = userInput.time.split(":");


When the user cancels the date, time seems to be an empty string:

{date: GregorianCalendarDate, time: ""}date: GregorianCalendarDate {calendarStart: CalendarDate, calendarEnd: CalendarDate, exactDate: true, jdnStart: 2458922, jdnEnd: 2458922, …}time: ""__proto__: Object

fixed in bfec41b

tobiasschweizer · 2020-03-12T11:34:12Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.spec.ts

+  }
+}
+
+describe('TimeInputComponent', () => {


Since you made separate functions for time conversion userInputToTimestamp and convertTimestampToDateTime, could add some tests for these? Just test the functions themselves, without any HTML/Dome interaction.

tests added in bfec41b

tobiasschweizer · 2020-03-12T11:34:59Z

@mdelez If you need my help, just give me a call.

…o a new PR

tobiasschweizer · 2020-03-12T13:39:40Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+
+    const date = new GregorianCalendarDate(new CalendarPeriod(calendarDate, calendarDate));
+
+    let time = this.datePipe.transform(timestamp, "HH:mm");


use const instead of let. Please use single quotes for strings.

Or just use WebStorm .. ;-)

fixed in e78d4cc I really tried to only use single quotes, but some slipped through 😜

tobiasschweizer · 2020-03-12T13:46:33Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+  @Input()
+  get value(): string | null {
+    const userInput = new DateTime(this.form.value.date, this.form.value.time);
+    if (userInput.date !== null && userInput.time !== null && userInput.time !== '') {


I think you should also check the time string against the regex here (to prevent an error in this.userInputToTimestamp ). If invalid, null is returned.

or probably you can just see if the FormControl for time is valid

fixed in 548005d

tobiasschweizer · 2020-03-16T07:47:33Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+
+  set value(timestamp: string | null) {
+    if (timestamp !== null) {
+      const dateTime = this.convertTimestampToDateTime(timestamp);


I think this should be wrapped in a try-catch block. If it fails to parse the timevalue string, you can still init {date: null, time: null}.

fixed in 19c3324

tobiasschweizer · 2020-03-16T08:05:25Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+  get value(): string | null {
+    if (this.form.valid) {
+      const userInput = new DateTime(this.form.value.date, this.form.value.time);
+      return this.userInputToTimestamp(userInput);


Could you wrap this with a try-catch block too (just return null if there is an error)?

fixed in 19c3324

tobiasschweizer · 2020-03-16T08:05:55Z

projects/knora-ui/src/lib/viewer/values/time-value/time-input/time-input.component.ts

+  userInputToTimestamp(userInput: DateTime): string {
+    const splitTime = userInput.time.split(':');
+    const updateDate = new Date(userInput.date.toCalendarPeriod().periodStart.year,
+                                (userInput.date.toCalendarPeriod().periodStart.month - 1),


Please add a comment about 0-based indexes for months

fixed in 19c3324

tobiasschweizer

Great, thx!

I think we still need to find out about the timezones, but there is #38 for this.

mdelez added 8 commits February 26, 2020 15:53

feature (value): added functionality for displaying and updating a ti…

0ccd561

…me value

feature (value): don't allow user to submit the same date

8fddcac

refactor (value): cleaned up some code and removed a ton of console logs

c32d125

feature (value): time validator added

f95ba71

feature (value): disabled user input for the date to ensure users ent…

61241d1

…er a correct date with the picker

refactor (value): removed some unnecessary code from the input template

d119e52

test (value): implementing unit tests

e533767

test (values): updated tests

3d3b1a7

mdelez added 3 commits March 3, 2020 15:15

test (value): local tests working

be65dfc

test (CI): trying to fix timezone issue

58eab34

test (CI): removed '-'

04513e9

mdelez mentioned this pull request Mar 4, 2020

Potential Timestamp Issue #38

Open

mdelez added 5 commits March 5, 2020 09:02

test (value): fixed local issue with time value tests

70251df

test (value): activating all tests

9ae3167

feature (value): added create new time value & tests

bc99455

refactor (value): removed some console logs

2ffe94e

feature (value & css): added some css to the time value component and…

847388d

… changed the comment input to a text area

mdelez requested review from tobiasschweizer and removed request for tobiasschweizer March 5, 2020 13:04

flavens reviewed Mar 6, 2020

View reviewed changes

mdelez added 3 commits March 6, 2020 15:38

feature (css): added icons for CRUD buttons and refactored some css t…

79a261b

…o remove as much deprecated styling as possible

refactor (test): added missing import in unit test

0d0a06e

refactor (css): reverted some css

ede712a

mdelez self-assigned this Mar 9, 2020

mdelez mentioned this pull request Mar 9, 2020

Value components CSS #39

Closed

mdelez requested a review from tobiasschweizer March 10, 2020 09:33

mdelez added 2 commits March 12, 2020 10:42

refactor (value): moved date logic and coonversion from the parent co…

046010e

…mponent to the child component and cleaned up a lot of repeated code

refactor (value): moved the date conversion logic into functions so t…

9afaa90

…hey can be more easily tested

mdelez requested a review from tobiasschweizer March 12, 2020 10:50

tobiasschweizer suggested changes Mar 12, 2020

View reviewed changes

tobiasschweizer reviewed Mar 12, 2020

View reviewed changes

mdelez added 3 commits March 12, 2020 13:43

refactor (value): code fixes and some extra tests

bfec41b

refactor (value): replaced type annotation

6392c37

refactor (value): removed all styling changes in order to move them t…

fcf1951

…o a new PR

mdelez requested a review from tobiasschweizer March 12, 2020 13:20

tobiasschweizer reviewed Mar 12, 2020

View reviewed changes

mdelez and others added 4 commits March 12, 2020 15:13

refactor (value): small fixes and changed double quotes to single quotes

e78d4cc

fix (value): set validators correctly

43ee1e8

refactor (value): trying to refactor tests

692226b

fix (values): fixed unit tests

548005d

mdelez requested a review from tobiasschweizer March 13, 2020 13:20

mdelez added 3 commits March 13, 2020 14:34

Merge branch 'master' into wip/16-time-value-component-2

44b3226

fix (value): fixed merge issues

ceaa15c

fix (values): fixed some wrong imports and removed some unused code

6b4ca83

tobiasschweizer suggested changes Mar 16, 2020

View reviewed changes

tobiasschweizer reviewed Mar 16, 2020

View reviewed changes

refactor (value): added some try-catch blocks

19c3324

tobiasschweizer approved these changes Mar 16, 2020

View reviewed changes

mdelez merged commit dc9c557 into master Mar 16, 2020

mdelez deleted the wip/16-time-value-component-2 branch March 16, 2020 08:44

		@@ -0,0 +1,3 @@
		::ng-deep .parent-value-component .mat-form-field {


		const date = new GregorianCalendarDate(new CalendarPeriod(calendarDate, calendarDate));

		let time = this.datePipe.transform(timestamp, "HH:mm");

Time Value Component #36

Time Value Component #36

Conversation

mdelez commented Feb 26, 2020

mdelez commented Mar 2, 2020

mdelez commented Mar 3, 2020

mdelez commented Mar 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasschweizer commented Mar 11, 2020

mdelez commented Mar 12, 2020

tobiasschweizer 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

tobiasschweizer commented Mar 12, 2020

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

tobiasschweizer Mar 16, 2020 • edited

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

tobiasschweizer left a comment

Choose a reason for hiding this comment

mdelez commented Mar 4, 2020 •

edited

tobiasschweizer Mar 16, 2020 •

edited