Add min-max values and length #275
Conversation
mihailkitrozoskicodechem
requested review from
DavidPavlovski,
ElenaStamenkovskaCC and
fvanderflier
and removed request for
DavidPavlovski
| } | ||
| } | ||
|
|
||
| static validMinField(control: AbstractControl): ValidationErrors | null { |
There was a problem hiding this comment.
I've noticed that the validators do not handle cases where characters other than numbers are provided. This input passes frontend validation without errors; however, it neither causes a backend failure nor results in the attribute being added to the model. While this is not expected to cause issues since the input is invalid, it would be preferable to validate this on the frontend before sending the request to the backend.
| @if (fieldForm.hasError('minGreaterThanMax') && | ||
| (fieldForm.get('minLength')?.touched || fieldForm.get('maxLength')?.touched)) { | ||
| <mat-error class="cross-field-error"> | ||
| Min length cannot be greater than max length |
There was a problem hiding this comment.
The error messages vary across the dialogs. It would be nice to follow a consistent design.
| var min = string.IsNullOrEmpty(minLength) ? "0" : minLength; | ||
| var max = string.IsNullOrEmpty(maxLength) ? DefaultMaxLength.ToString() : maxLength; | ||
|
|
||
| var attribute = SyntaxFactory.Attribute(SyntaxFactory.ParseName("Length")); |
There was a problem hiding this comment.
Why do we always create them with [Length]? If I provide only maxLength I would expect to have only the [MaxLength] attribute and same goes for [MinLength].
| min="1" | ||
| max="1000" | ||
| step="1" | ||
| formControlName="minLength" |
There was a problem hiding this comment.
There is a missing placeholder for minLength.
| ...(this.selectedFieldTypeId === this.fieldTypeEnum.String && this.fieldForm.value.minLength && { minLength: this.fieldForm.value.minLength }), | ||
| ...(this.selectedFieldTypeId === this.fieldTypeEnum.String && this.fieldForm.value.maxLength && { maxLength: this.fieldForm.value.maxLength }), | ||
| ...((this.selectedFieldTypeId === this.fieldTypeEnum.Number || this.selectedFieldTypeId === this.fieldTypeEnum.Float || this.selectedFieldTypeId === this.fieldTypeEnum.Double) && this.fieldForm.value.minValue !== null && this.fieldForm.value.minValue !== undefined && this.fieldForm.value.minValue !== '' && { minValue: this.fieldForm.value.minValue }), | ||
| ...((this.selectedFieldTypeId === this.fieldTypeEnum.Number || this.selectedFieldTypeId === this.fieldTypeEnum.Float || this.selectedFieldTypeId === this.fieldTypeEnum.Double) && this.fieldForm.value.maxValue !== null && this.fieldForm.value.maxValue !== undefined && this.fieldForm.value.maxValue !== '' && { maxValue: this.fieldForm.value.maxValue }) |
There was a problem hiding this comment.
Could you refactor this by extracting parts into separate methods and introducing boolean variables or similar constructs? The current implementation is becoming increasingly complex, making it difficult to follow the logic.
| (this.isTextType() && formValue.regex !== (this.data.regex || '')) || | ||
| (this.isDateType() && formValue.noPastDates !== (this.data.noPastDates || false)) | ||
| (this.isDateType() && formValue.noPastDates !== (this.data.noPastDates || false)) || | ||
| (this.isTextType() && (formValue.minLength !== (this.data.minLength ?? null) || formValue.maxLength !== (this.data.maxLength ?? null))) || |
There was a problem hiding this comment.
Similar note to the previous one, try to take out these in separate variables.
| hasIndex?: boolean; | ||
| regex?: string; | ||
| noPastDates?: boolean; | ||
| minLength?: number | null; |
There was a problem hiding this comment.
Why do we need explicit nullability here? The optional property already covers undefined.
| hasIndex: boolean; | ||
| regex?: string; | ||
| noPastDates?: boolean; | ||
| minLength?: number | null; |
There was a problem hiding this comment.
Same question as before, the optional property covers undefined. Make sure you have consistent code everywhere.
|
|
||
| public int? MaxLength { get; set; } | ||
|
|
||
| public double? MinValue { get; set; } |
There was a problem hiding this comment.
The MinValue and MaxValue here are of type double. While in Property.cs they are strings. Was this intentional?
Implements the ability for users to specify minimum and maximum values on number and text properties.
Closes #222