Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Added required input for single file attachment #78

Merged
merged 10 commits into from
Nov 18, 2019
31 changes: 6 additions & 25 deletions src/app/public/modules/checkbox/checkbox.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@ import {
} from '@angular/core';

import {
AbstractControl,
ControlValueAccessor,
NgControl
} from '@angular/forms';

import {
SkyFormsUtility
} from '../shared/forms-utility';

/**
Blackbaud-AlexKingman marked this conversation as resolved.
Show resolved Hide resolved
* Monotonically increasing integer used to auto-generate unique ids for checkbox components.
*/
Expand Down Expand Up @@ -101,7 +104,7 @@ export class SkyCheckboxComponent implements ControlValueAccessor, OnInit {
*/
@Input()
set required(value: boolean) {
this._required = this.coerceBooleanProperty(value);
this._required = SkyFormsUtility.coerceBooleanProperty(value);
}

get required(): boolean {
Expand All @@ -127,7 +130,7 @@ export class SkyCheckboxComponent implements ControlValueAccessor, OnInit {
public ngOnInit(): void {
if (this.ngControl) {
// Backwards compatibility support for anyone still using Validators.Required.
this.required = this.required || this.hasRequiredValidation(this.ngControl);
this.required = this.required || SkyFormsUtility.hasRequiredValidation(this.ngControl);
}
}

Expand Down Expand Up @@ -201,26 +204,4 @@ export class SkyCheckboxComponent implements ControlValueAccessor, OnInit {
this.checked = !this.checked;
}

/**
* Gets the required state of the checkbox.
* Currently, Angular doesn't offer a way to get this easily, so we have to create an empty
* control using the current validator to see if it throws a `required` validation error.
* https://github.com/angular/angular/issues/13461#issuecomment-340368046
*/
private hasRequiredValidation(ngControl: NgControl): boolean {
const abstractControl = ngControl.control as AbstractControl;
if (abstractControl && abstractControl.validator) {
const validator = abstractControl.validator({} as AbstractControl);
if (validator && validator.required) {
return true;
}
}
return false;
}

/** Coerces a data-bound value (typically a string) to a boolean. */
private coerceBooleanProperty(value: any): boolean {
return value !== undefined && `${value}` !== 'false';
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<div
class="sky-file-attachment-label-wrapper"
[ngClass]="{ 'sky-control-label-required': required && hasLabelComponent }"
[attr.aria-required]="(required) ? true : null"
[attr.id]="labelElementId"
>
<ng-container *ngTemplateOutlet="labelContent"
Expand All @@ -22,6 +23,7 @@
tabindex="-1"
type="file"
[attr.accept]="acceptedTypes || null"
[required]="required"
(change)="fileChangeEvent($event)"
#fileInput
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ describe('File attachment', () => {
fileAttachmentInstance = fixture.componentInstance.fileAttachmentComponent;
});

//#region helpers
function getInputDebugEl(): DebugElement {
return fixture.debugElement.query(By.css('input'));
}
Expand Down Expand Up @@ -304,18 +305,54 @@ describe('File attachment', () => {
}
//#endregion

it('should allow the user to specify if the file is required', fakeAsync(() => {
it('should not have required class and aria-reqiured attribute when not required', fakeAsync(() => {
fileAttachmentInstance.ngAfterViewInit();
tick();
fixture.detectChanges();

let labelWrapper = getLabelWrapper();
// expect(labelWrapper.getAttribute('required')).toBeNull(); IS THIS RIGHT? Shouldn't it be on the input?
expect(labelWrapper.classList.contains('sky-control-label-required')).toBe(false);
expect(labelWrapper.getAttribute('aria-required')).toBeNull();
}));

it('should have appropriate classes when file is required', fakeAsync(() => {
fixture.componentInstance.required = true;
fileAttachmentInstance.ngAfterViewInit();
tick();
fixture.detectChanges();

let labelWrapper = getLabelWrapper();

// expect(fileAttachmentInstance.required).toBe(true);
expect(labelWrapper.classList.contains('sky-control-label-required')).toBe(true);
expect(labelWrapper.getAttribute('aria-required')).toBe('true');
}));

it('should have appropriate classes when file is required and initialized with file', fakeAsync(() => {
fixture.componentInstance.required = true;
const testFile = <SkyFileItem> {
file: {
name: 'myFile',
type: '',
size: 1
},
url: 'myFile'
};
fileAttachmentInstance.value = testFile;
fileAttachmentInstance.ngAfterViewInit();
tick();
fixture.detectChanges();

let labelWrapper = getLabelWrapper();

expect(fileAttachmentInstance.required).toBe(true);
// expect(fileAttachmentInstance.required).toBe(true);
expect(labelWrapper.classList.contains('sky-control-label-required')).toBe(true);
expect(labelWrapper.getAttribute('aria-required')).toBe('true');
}));

it('should handle removing the label', fakeAsync(() => {
fixture.componentInstance.required = true;
fileAttachmentInstance.ngAfterViewInit();
fileAttachmentInstance.ngAfterContentInit();
tick();
Expand Down
75 changes: 31 additions & 44 deletions src/app/public/modules/file-attachment/file-attachment.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,27 @@ import {
ContentChildren,
ElementRef,
EventEmitter,
forwardRef,
Input,
OnDestroy,
Optional,
Output,
ViewChild,
QueryList
QueryList,
Self,
ViewChild
} from '@angular/core';

import {
ControlValueAccessor,
AbstractControl,
NG_VALUE_ACCESSOR,
NG_VALIDATORS
NgControl
} from '@angular/forms';

import {
Subject
} from 'rxjs';

import {
SkyFormsUtility
} from '../shared/forms-utility';

import {
SkyFileAttachmentChange
} from './types/file-attachment-change';
Expand All @@ -50,31 +52,16 @@ import {
SkyFileItemService
} from './file-item.service';

// tslint:disable:no-forward-ref no-use-before-declare
const SKY_FILE_ATTACHMENT_VALUE_ACCESSOR = {
provide: NG_VALUE_ACCESSOR,
useExisting: forwardRef(() => SkyFileAttachmentComponent),
multi: true
};

const SKY_FILE_ATTACHMENT_VALIDATOR = {
provide: NG_VALIDATORS,
useExisting: forwardRef(() => SkyFileAttachmentComponent),
multi: true
};

Blackbaud-SteveBrush marked this conversation as resolved.
Show resolved Hide resolved
let uniqueId = 0;

@Component({
selector: 'sky-file-attachment',
templateUrl: './file-attachment.component.html',
styleUrls: ['./file-attachment.component.scss'],
providers: [
SKY_FILE_ATTACHMENT_VALUE_ACCESSOR,
SKY_FILE_ATTACHMENT_VALIDATOR
],
changeDetection: ChangeDetectionStrategy.OnPush
})
export class SkyFileAttachmentComponent implements ControlValueAccessor, AfterViewInit, AfterContentInit, OnDestroy {
export class SkyFileAttachmentComponent implements AfterViewInit, AfterContentInit, OnDestroy {

@Input()
public acceptedTypes: string;

Expand Down Expand Up @@ -105,6 +92,10 @@ export class SkyFileAttachmentComponent implements ControlValueAccessor, AfterVi

public rejectedOver: boolean = false;

/**
* Indicates if the input must have a file to be valid. This property accepts boolean values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by "to be valid." What is considered valid? The single file attachment? Or the form that it is included on? I mean, is the validation specific to the single file attachment? Or is it specific to a form, to indicate that users must attach a file before they can complete the form? The latter makes more sense to me, but the current wording seems like the validation is specific to the input. Also, in #77, we explain the impact of setting required to true. Is there any reason not to include that information in the description? Assuming my understanding of all of this is correct, what do you think of fleshing out this description to:

"Indicates whether the input is required for form validation. When you set this property to true, the component adds aria-required and required attributes to the input element, and both template-driven forms and reactive forms show an invalid state until the input element is complete. This property accepts a boolean value."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's all accurate, although I'm not sure we have to point out that both reactive and template-driven forms are supported. Isn't that just the assumption that we would support them both?

Copy link
Contributor

@blackbaud-johnly blackbaud-johnly Nov 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The description in #77 called out both reactive and template-driven forms, and I included it here without fully thinking it through. Since we don't really need to call that out, we can tighten up to:

"Indicates whether the input is required for form validation. When you set this property to true, the component adds aria-required and required attributes to the input element so that forms display an invalid state until the input element is complete. This property accepts a boolean value."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

*/
@Input()
public required: boolean = false;

public set value(value: SkyFileItem) {
Expand All @@ -130,17 +121,24 @@ export class SkyFileAttachmentComponent implements ControlValueAccessor, AfterVi
@ContentChildren(SkyFileAttachmentLabelComponent)
private labelComponents: QueryList<SkyFileAttachmentLabelComponent>;

private control: AbstractControl;
private enterEventTarget: any;

private fileAttachmentId = uniqueId++;

private ngUnsubscribe = new Subject<void>();

private _value: any;

constructor(
private changeDetector: ChangeDetectorRef,
private fileAttachmentService: SkyFileAttachmentService,
private fileItemService: SkyFileItemService
) { }
private fileItemService: SkyFileItemService,
@Self() @Optional() private ngControl: NgControl
) {
if (this.ngControl) {
this.ngControl.valueAccessor = this;
}
}

public ngAfterViewInit(): void {
// This is needed to address a bug in Angular 7.
Expand All @@ -149,19 +147,16 @@ export class SkyFileAttachmentComponent implements ControlValueAccessor, AfterVi
// Of note is the parent check which allows us to determine if the form is reactive.
// Without this check there is a changed before checked error
/* istanbul ignore else */
if (this.control) {
if (this.ngControl) {
setTimeout(() => {
this.control.setValue(this.value, {
this.ngControl.control.setValue(this.value, {
emitEvent: false
});

// Set required to apply required state to label
if (this.control.errors && this.control.errors.required) {
this.required = true;
}

this.changeDetector.markForCheck();
});

// Backwards compatibility support for anyone still using Validators.Required.
this.required = this.required || SkyFormsUtility.hasRequiredValidation(this.ngControl);
}
}

Expand Down Expand Up @@ -286,14 +281,6 @@ export class SkyFileAttachmentComponent implements ControlValueAccessor, AfterVi
this.changeDetector.markForCheck();
}

public validate(control: AbstractControl): { [key: string]: any } {
if (!this.control) {
this.control = control;
}

return undefined;
}

Blackbaud-SteveBrush marked this conversation as resolved.
Show resolved Hide resolved
public emitClick(): void {
this.fileClick.emit({
file: this.value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
>
<sky-file-attachment
formControlName="attachment"
[required]="required"
>
<sky-file-attachment-label
*ngIf="showLabel"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,31 @@
import {
Component,
ViewChild,
OnInit
OnInit,
ViewChild
} from '@angular/core';

import {
FormBuilder,
FormControl,
FormGroup
} from '@angular/forms';

import {
SkyFileAttachmentComponent
} from '../file-attachment.component';

import { FormGroup, FormControl, Validators, FormBuilder } from '@angular/forms';

@Component({
selector: 'file-attachment-test',
templateUrl: './file-attachment.component.fixture.html'
})
export class FileAttachmentTestComponent implements OnInit {

public attachment: FormControl;

public fileForm: FormGroup;

public required: boolean = false;

public showLabel: boolean = true;

@ViewChild(SkyFileAttachmentComponent)
Expand All @@ -29,7 +36,7 @@ export class FileAttachmentTestComponent implements OnInit {
) { }

public ngOnInit(): void {
this.attachment = new FormControl(undefined, Validators.required);
this.attachment = new FormControl(undefined);
this.fileForm = this.formBuilder.group({
attachment: this.attachment
});
Expand Down
32 changes: 32 additions & 0 deletions src/app/public/modules/shared/forms-utility.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import {
AbstractControl,
NgControl
} from '@angular/forms';

// Need to add the following to classes which contain static methods.
// See: https://github.com/ng-packagr/ng-packagr/issues/641
// @dynamic
export class SkyFormsUtility {

/** Coerces a data-bound value (typically a string) to a boolean. */
public static coerceBooleanProperty(value: any): boolean {
return value !== undefined && `${value}` !== 'false';
}

/**
* Gets the required state of the checkbox.
* Currently, Angular doesn't offer a way to get this easily, so we have to create an empty
* control using the current validator to see if it throws a `required` validation error.
* https://github.com/angular/angular/issues/13461#issuecomment-340368046
*/
public static hasRequiredValidation(ngControl: NgControl): boolean {
const abstractControl = ngControl.control as AbstractControl;
if (abstractControl && abstractControl.validator) {
const validator = abstractControl.validator({} as AbstractControl);
if (validator && validator.required) {
return true;
}
}
return false;
}
}
Loading