Fix async disable selection box #280

bradenbiz · 2021-07-14T17:23:31Z

Bug:

disabling a selection box form control after initialization of the components led to some bad-looking css bugs.

Fix:

make sure each of the internal components (checkbox & radio) are properly passing the disabled state of the form control to each of the outer components (selection box), so that the css can be updated properly.

Changes made:

src/app/public/modules/checkbox/checkbox.component.spec.ts

modify SingleCheckboxComponent to have a ViewChild that grabs the SkyCheckboxComponent so that we can subscribe() to disabledChange values and call onDisabledChange() every time
write new test to spyOn() how many times onDisabledChange() is called, and use that to determine if the SkyCheckboxComponent is properly emitting new disabled values

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

use a getter pattern for the disabled variable
create the disabledChange emitter

src/app/public/modules/radio/fixtures/radio.component.fixture.html

move the disabling behavior from the second radio button to the first so to make the new test work as expected

src/app/public/modules/radio/fixtures/radio.component.fixture.ts

add a ViewChild that grabs the SkyRadioComponent so that we can subscribe() to disabledChange values and call onDisabledChange() every time

src/app/public/modules/radio/radio.component.spec.ts

write new test to spyOn() how many times onDisabledChange() is called, and use that to determine if the SkyRadioComponent is properly emitting new disabled values
modify existing test to work as it used to, but with the radio fixture's HTML changes

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

use a getter pattern for the disabled variable
create the disabledChange emitter

src/app/public/modules/selection-box/fixtures/selection-box-fixtures.module.ts

import the ReactiveFormsModule so to allow reactive forms to work in the selection box fixture

src/app/public/modules/selection-box/fixtures/selection-box.component.fixture.html

wrap the sky-selection-box components in a form
use the newly created FormControls to loop through the creation of the selection boxes
set up each of the checkbox components with a form control value

src/app/public/modules/selection-box/fixtures/selection-box.component.fixture.ts

rename variables
create and initialize the FormGroup as a FormArray with FormControls
create a getter for the new FormArray

src/app/public/modules/selection-box/selection-box.component.html

code styling

src/app/public/modules/selection-box/selection-box.component.spec.ts

write new test to disable/enable the selection and confirm that the proper css class has been added to it

src/app/public/modules/selection-box/selection-box.component.ts

markForCheck() every time disabled is set
subscribe() to disabledChange and update disabled every time a new value is pushed

src/app/visual/selection-box/selection-box-visual.component.html

wrap the sky-selection-box components in a form
use the newly created FormControls to loop through the creation of the selection boxes
set up each of the checkbox components with a form control value

src/app/visual/selection-box/selection-box-visual.component.ts

rename variables
create and initialize the FormGroup as a FormArray with FormControls
create a getter for the new FormArray

…... WIP for Alex's review

…o bradensBrokenFixture

blackbaud-ado · 2021-07-14T17:46:56Z

Preview build at https://developer.blackbaud.com/skyux-forms?_sp.spa=pr-fixAsyncDisableSelectionBox

Blackbaud-AlexKingman

Great work on this! Just a few comments, but mostly code style stuff. Check out our guide here for more info: https://host.nxt.blackbaud.com/skyux-team-docs/style

Also, one last question: do we have a visual test that covers the disabled state? I'm pretty sure we do, just want to double check.

src/app/public/modules/checkbox/checkbox.component.spec.ts

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

src/app/public/modules/radio/radio.component.spec.ts

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

src/app/public/modules/selection-box/fixtures/selection-box-fixtures.module.ts

src/app/public/modules/selection-box/fixtures/selection-box.component.fixture.ts

src/app/public/modules/selection-box/selection-box.component.spec.ts

src/app/public/modules/selection-box/selection-box.component.ts

bradenbiz · 2021-07-14T21:15:26Z

In reference to your question of if we have a visual test for disabling of the component, I believe this test is what you are looking for:

skyux-forms/e2e/selection-box.e2e-spec.ts

Lines 120 to 127 in 63bdf08

    
           it('should match previous screenshot when disabled', async (done) => { 
        
             await SkyHostBrowser.scrollTo('#screenshot-selection-box-disabled'); 
        
             await clickCheckbox(); 
        
             expect('#screenshot-selection-box-disabled') 
        
               .toMatchBaselineScreenshot(done, { 
        
                 screenshotName: getScreenshotName('selection-box-disabled') 
        
               }); 
        
           });

blackbaud-ado · 2021-07-14T21:20:59Z

Preview build at https://developer.blackbaud.com/skyux-forms?_sp.spa=pr-fixAsyncDisableSelectionBox

codecov · 2021-07-14T22:13:10Z

Codecov Report

Merging #280 (88b8211) into master (63bdf08) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #280   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           37        37           
  Lines         1081      1098   +17     
  Branches       209       212    +3     
=========================================
+ Hits          1081      1098   +17

Impacted Files	Coverage Δ
.../app/public/modules/checkbox/checkbox.component.ts	`100.00% <100.00%> (ø)`
src/app/public/modules/radio/radio.component.ts	`100.00% <100.00%> (ø)`
...c/modules/selection-box/selection-box.component.ts	`100.00% <100.00%> (ø)`

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 63bdf08...88b8211. Read the comment docs.

blackbaud-ado · 2021-07-15T16:07:58Z

Preview build at https://developer.blackbaud.com/skyux-forms?_sp.spa=pr-fixAsyncDisableSelectionBox

Blackbaud-AlexKingman · 2021-07-16T15:07:12Z

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

@@ -126,7 +140,7 @@ export class SkyCheckboxComponent implements ControlValueAccessor, OnInit {
  }

  /**
-   * Indicates whether the checkbox is selected.
+   * Indicates whether to the checkbox is selected.


Did this to sneak in here accidentally?

good catch, fixed!

Blackbaud-AlexKingman · 2021-07-16T15:09:22Z

src/app/visual/checkbox/checkbox-visual.component.html

-    <sky-checkbox-label>Disabled Checkbox</sky-checkbox-label>
+  <sky-checkbox
+    [checked]="false"
+    [disabled]="true">


Closing bracket > needs to be on a new line.

weird... this is fixed locally, and it says i pushed my commit for that file. ill try pushing again

nevermind, misread your comment. fixed in last commit!

blackbaud-ado · 2021-07-16T15:11:29Z

Preview build at https://developer.blackbaud.com/skyux-forms?_sp.spa=pr-fixAsyncDisableSelectionBox

Blackbaud-AlexKingman

🚀 great job!

blackbaud-ado · 2021-07-16T15:29:36Z

Preview build at https://developer.blackbaud.com/skyux-forms?_sp.spa=pr-fixAsyncDisableSelectionBox

* Input box inline help (#265) * initial work * character-count * padding tweaks * build fix * fixed merge * final positioning for inline help widget * visual test tweak, test fix * style tweaks * updated selector to use sky-control-help Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Build #skyux-forms-push-929062241-56873: Added new baseline screenshots. [ci skip] * Update changelog/package.json for 4.19.0 release (#270) Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Fix single file attachment to show full file name in title attribute (#271) * Update changelog/package.json for 4.19.1 release (#272) * Added left inset icon for input-box; fix padding bug with country field (#273) * Added left inset icon for input-box * visual tweaks * unit tests * added scss variables * fix country field spacing Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Build #skyux-forms-push-990175149-08428: Added new baseline screenshots. [ci skip] * 4.20.0 release (#274) * 4.20.0 release * package lock * Update CHANGELOG.md Co-authored-by: Paula Hodgkins <77297281+blackbaud-paulah@users.noreply.github.com> Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Paula Hodgkins <77297281+blackbaud-paulah@users.noreply.github.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Fix docs for checkbox (#276) * Fix async disable selection box (#280) * all changes associated to trying to fix the selection box disable bug... WIP for Alex's review * complete disable selection-box unit test. fix broken radio disable test. * remove unnecesary local experimentation leftovers * typo * fixing things according to Alex's comments * remove editor ruler and word wrap * fix template style guide violations * add disable/enable button to /visual selection-box component * correct typo and a style guide violation * missed style guide violation * update changelog. skyux upgrade. (#282) * update changelog. skyux upgrade. * Update CHANGELOG.md Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Update CHANGELOG.md Co-authored-by: John Lyons <John.Lyons@blackbaud.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Fix overlapping input box value and label for textareas in modern theme (#279) * Fix overlapping input box value and label for textareas in modern theme * reverted accidental comments * removed baselines to allow for tiny shift in padding * fixed visual test * fix e2e test * e2e fix Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Paul Crowder <paul.crowder@blackbaud.com> * Build #skyux-forms-push-1049895399-36432: Added new baseline screenshots. [ci skip] * Changelog update; redo 4.20.1 release (#283) * changelog update * Update CHANGELOG.md Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Update CHANGELOG.md Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Removed accidental disabled e2e tests for input box (#284) Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Alex <alex.kingman@blackbaud.com> Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Blackbaud Sky Build User <sky-build-user@blackbaud.com> Co-authored-by: Steve Brush <steve.brush@blackbaud.com> Co-authored-by: Paula Hodgkins <77297281+blackbaud-paulah@users.noreply.github.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> Co-authored-by: Braden Bisping <63625452+bradenbiz@users.noreply.github.com>

* initial commit * Updated singletons to use providedIn: root (#269) Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Merge from master (#275) Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * provider updates, dependency updates, workflow updates (#277) * provider updates, dependency updates, workflow updates * revert Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * 5.0.0-beta.0 release (#278) Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Deprecate SkySelectionBoxGridAlignItems (#281) * Deprecate SkySelectionBoxGridAlignItems * Export union type * Merge master into 5.0.0-next (#285) * Input box inline help (#265) * initial work * character-count * padding tweaks * build fix * fixed merge * final positioning for inline help widget * visual test tweak, test fix * style tweaks * updated selector to use sky-control-help Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Build #skyux-forms-push-929062241-56873: Added new baseline screenshots. [ci skip] * Update changelog/package.json for 4.19.0 release (#270) Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Fix single file attachment to show full file name in title attribute (#271) * Update changelog/package.json for 4.19.1 release (#272) * Added left inset icon for input-box; fix padding bug with country field (#273) * Added left inset icon for input-box * visual tweaks * unit tests * added scss variables * fix country field spacing Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Build #skyux-forms-push-990175149-08428: Added new baseline screenshots. [ci skip] * 4.20.0 release (#274) * 4.20.0 release * package lock * Update CHANGELOG.md Co-authored-by: Paula Hodgkins <77297281+blackbaud-paulah@users.noreply.github.com> Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Paula Hodgkins <77297281+blackbaud-paulah@users.noreply.github.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Fix docs for checkbox (#276) * Fix async disable selection box (#280) * all changes associated to trying to fix the selection box disable bug... WIP for Alex's review * complete disable selection-box unit test. fix broken radio disable test. * remove unnecesary local experimentation leftovers * typo * fixing things according to Alex's comments * remove editor ruler and word wrap * fix template style guide violations * add disable/enable button to /visual selection-box component * correct typo and a style guide violation * missed style guide violation * update changelog. skyux upgrade. (#282) * update changelog. skyux upgrade. * Update CHANGELOG.md Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Update CHANGELOG.md Co-authored-by: John Lyons <John.Lyons@blackbaud.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Fix overlapping input box value and label for textareas in modern theme (#279) * Fix overlapping input box value and label for textareas in modern theme * reverted accidental comments * removed baselines to allow for tiny shift in padding * fixed visual test * fix e2e test * e2e fix Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Paul Crowder <paul.crowder@blackbaud.com> * Build #skyux-forms-push-1049895399-36432: Added new baseline screenshots. [ci skip] * Changelog update; redo 4.20.1 release (#283) * changelog update * Update CHANGELOG.md Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Update CHANGELOG.md Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Removed accidental disabled e2e tests for input box (#284) Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Alex <alex.kingman@blackbaud.com> Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Blackbaud Sky Build User <sky-build-user@blackbaud.com> Co-authored-by: Steve Brush <steve.brush@blackbaud.com> Co-authored-by: Paula Hodgkins <77297281+blackbaud-paulah@users.noreply.github.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> Co-authored-by: Braden Bisping <63625452+bradenbiz@users.noreply.github.com> * Release 5.0.0-beta.1 (#286) * Next > Adopt Angular CLI (#288) * Release 5.0.0-beta.2 (#289) * Build #skyux-forms-push-1198364695-34681: Added new baseline screenshots. [ci skip] * Remove circular references (#290) * Fix visual tests * Fix import paths * Update packages * Delete screenshots * Build #skyux-forms-push-1210649827-68966: Added new baseline screenshots. [ci skip] * Release 5.0.0-beta.3 (#291) * Next > Update peer dependencies (#292) * Build #skyux-forms-push-1221916356-27461: Added new baseline screenshots. [ci skip] * 5.0.0-beta.5 release; removed unnecessary dependency on @skyux/modals (#293) * Removed modals * 5.0.0-beta.5-release * Update CHANGELOG.md * baselines Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Build #skyux-forms-push-1234569693-21840: Added new baseline screenshots. [ci skip] * Document sky-control-help class (#295) * Document sky-control-help class * Update dependencies as per error messages * package.lock * reset baselines Co-authored-by: John Lyons <John.Lyons@blackbaud.me> Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> * Build #skyux-forms-push-1258643898-83004: Added new baseline screenshots. [ci skip] * Workflow and package updates * Fix tests * Reset screenshot * Reset screenshot * Exact dependencies * @skyux/packages as prod dependency * Reset screenshots * Fix tests * Reset screenshots Co-authored-by: Alex Kingman <alex.kingman@blackbaud.me> Co-authored-by: Alex <alex.kingman@blackbaud.com> Co-authored-by: Paul Crowder <paul.crowder@blackbaud.com> Co-authored-by: Blackbaud Sky Build User <sky-build-user@blackbaud.com> Co-authored-by: Steve Brush <steve.brush@blackbaud.com> Co-authored-by: Paula Hodgkins <77297281+blackbaud-paulah@users.noreply.github.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> Co-authored-by: Braden Bisping <63625452+bradenbiz@users.noreply.github.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.me>

bradenbisping added 4 commits July 13, 2021 09:27

all changes associated to trying to fix the selection box disable bug…

629fe7c

…... WIP for Alex's review

Merge branch 'master' of https://github.com/blackbaud/skyux-forms int…

618bb66

…o bradensBrokenFixture

complete disable selection-box unit test. fix broken radio disable test.

e973ffe

remove unnecesary local experimentation leftovers

8f27fc6

bradenbiz requested a review from Blackbaud-AlexKingman July 14, 2021 17:24

typo

56d4472

Blackbaud-AlexKingman reviewed Jul 14, 2021

View reviewed changes

fixing things according to Alex's comments

2f12845

remove editor ruler and word wrap

23f046d

bradenbisping added 2 commits July 16, 2021 09:44

fix template style guide violations

b0f53cf

add disable/enable button to /visual selection-box component

6ffd240

Blackbaud-AlexKingman reviewed Jul 16, 2021

View reviewed changes

bradenbisping added 2 commits July 16, 2021 10:13

correct typo and a style guide violation

ad729ff

missed style guide violation

88b8211

Blackbaud-AlexKingman approved these changes Jul 16, 2021

View reviewed changes

bradenbiz merged commit 816a176 into master Jul 16, 2021

bradenbiz deleted the fixAsyncDisableSelectionBox branch July 16, 2021 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix async disable selection box #280

Fix async disable selection box #280

bradenbiz commented Jul 14, 2021

blackbaud-ado commented Jul 14, 2021

Blackbaud-AlexKingman left a comment •

edited

bradenbiz commented Jul 14, 2021

blackbaud-ado commented Jul 14, 2021

codecov bot commented Jul 14, 2021 •

edited

blackbaud-ado commented Jul 15, 2021

Blackbaud-AlexKingman Jul 16, 2021

bradenbiz Jul 16, 2021

Blackbaud-AlexKingman Jul 16, 2021

bradenbiz Jul 16, 2021

bradenbiz Jul 16, 2021

blackbaud-ado commented Jul 16, 2021

Blackbaud-AlexKingman left a comment •

edited

blackbaud-ado commented Jul 16, 2021

Fix async disable selection box #280

Fix async disable selection box #280

Conversation

bradenbiz commented Jul 14, 2021

Bug:

Fix:

Changes made:

blackbaud-ado commented Jul 14, 2021

Blackbaud-AlexKingman left a comment • edited

Choose a reason for hiding this comment

bradenbiz commented Jul 14, 2021

blackbaud-ado commented Jul 14, 2021

codecov bot commented Jul 14, 2021 • edited

Codecov Report

blackbaud-ado commented Jul 15, 2021

Blackbaud-AlexKingman Jul 16, 2021

Choose a reason for hiding this comment

bradenbiz Jul 16, 2021

Choose a reason for hiding this comment

Blackbaud-AlexKingman Jul 16, 2021

Choose a reason for hiding this comment

bradenbiz Jul 16, 2021

Choose a reason for hiding this comment

bradenbiz Jul 16, 2021

Choose a reason for hiding this comment

blackbaud-ado commented Jul 16, 2021

Blackbaud-AlexKingman left a comment • edited

Choose a reason for hiding this comment

blackbaud-ado commented Jul 16, 2021

Blackbaud-AlexKingman left a comment •

edited

codecov bot commented Jul 14, 2021 •

edited

Blackbaud-AlexKingman left a comment •

edited