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

Added required input in checkbox component #77

Merged
merged 13 commits into from
Nov 14, 2019

Conversation

Blackbaud-AlexKingman
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman commented Oct 31, 2019

New @Input() property:

required - Indicates if the checkbox must be checked to be valid. This property accepts a boolean values.

Impact of setting required to true:

  • aria-required attribute added to input element
  • required attribute added to input element
  • Angular form should show invalid state when checkbox isn't checked

This works with both template-driven & reactive forms.

@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #77   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          23     24    +1     
  Lines         877    895   +18     
  Branches      119    124    +5     
=====================================
+ Hits          877    895   +18
Impacted Files Coverage Δ
.../app/public/modules/checkbox/checkbox.component.ts 100% <100%> (ø) ⬆️
src/app/public/modules/checkbox/checkbox.module.ts 100% <100%> (ø) ⬆️
.../checkbox/checkbox-required-validator.directive.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 996c18b...19b77b2. Read the comment docs.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Looks good! Just a small comment.

src/app/visual/checkbox/checkbox-visual.component.html Outdated Show resolved Hide resolved
src/app/public/modules/checkbox/checkbox.component.ts Outdated Show resolved Hide resolved

if (this.required && !control.value) {
return {
'required': true

Choose a reason for hiding this comment

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

If I check the checkbox it becomes "valid", but if I uncheck it, it remains "valid". Shouldn't it be invalid?

Copy link
Contributor Author

@Blackbaud-AlexKingman Blackbaud-AlexKingman Oct 31, 2019

Choose a reason for hiding this comment

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

Yes, but I'm unable to replicate this. We also have two unit tests that check for this in both reactive and T-D. Are you doing this with the visual demo, or did you make your own component?

Choose a reason for hiding this comment

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

I encountered this on the visual demo.

@Blackbaud-AlexKingman Blackbaud-AlexKingman changed the title Added support for required attribute in checkbox component Added required input in checkbox component Nov 1, 2019
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

<form
  [formGroup]="reactiveFormGroup"
  novalidate
>
  <sky-checkbox
    formControlName="reactiveCheckbox"
  >
    <sky-checkbox-label>
      Check me
    </sky-checkbox-label>
  </sky-checkbox>
</form>
this.reactiveFormGroup = this.formBuilder.group(
  {
    reactiveCheckbox: new FormControl(undefined, Validators.required)
  }
);

When I use the above setup, a checkbox is still (incorrectly) considered valid when I check and uncheck it. This works fine when using the input, however. Disregard; this works with the Validators.requiredTrue!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants