VA Risk Wizard step two #370

CloudNiner · 2017-12-22T16:45:19Z

Overview

Adds functioning step two of the Risk Wizard in the Vulnerability Assessment.

Demo

Blank form for new risk (continue disabled)

OptionDropdown open to select probability

After selecting other items (continue enabled once one form control changed)

Continue to next step (selections printed to console.log on navigation)

Back to previous step (form selections saved)

Notes

I added models for the Directional and Relative options sent by the API. The enum/maps for each of those felt like the most straightforward, if a bit verbose way to get proper typecasting. It's not possible to use a string enum (effectively a typed Object) as a standard JS Object key until typescript 2.6. As a result, maps are a bit more verbose than objects, but effectively the same.

I added the OptionDropdownComponent in the SharedModule. Should be ready for use by any other app components that need it.

I renamed the to/from abstract handler methods in WizardSessionService to use "model" instead of "data" for a bit more clarity about which method is which. I also removed all remaining references to Risk in the WizardSessionService and WizardStepComponent.

In an effort to be continually refactoring, I moved scss files for components that are declared in the SharedModule to a new folder shared inside the sass components directory.

I added some generic classes for each wizard step to use, see identify and hazard step HTML for examples, each wizard step now has a step-header, step-content and step-footer class. Added some basic padding to make the step HTML elements not so scrunched, these can be updated, tweaked, whatevered by @alexelash later.

I updated the archwizard to color the navigation step circles in the wizard nav sidebar to match what (I think) made sense for our application. Again, these can be updated by @alexelash later.

Assigned @fungjj92 because she'll be here next week, but probably worth it for @maurizi and @flibbertigibbet to at least take a look as well. The models added here for the API choices should unblock fixing the adaptive need boxes as part of #322.

Testing Instructions

Follow the workflow in the demo section
"Question mark" tooltips should work too

Closes #236

Adds and use models for HazardStep form controls: - OrgRiskRelativeOption - OrgRiskDirectionalOption

fungjj92

Overall lookin good! Thanks for providing ample explanation in your PR post and in code comments. See my comments!

fungjj92 · 2017-12-22T18:14:23Z

src/angular/planit/src/app/risk-wizard/risk-wizard.component.ts

@@ -29,8 +29,12 @@ export class RiskWizardComponent implements AfterViewInit, OnDestroy, OnInit {

  ngOnInit() {
    // TODO: Set initial risk from API


Is there an issue you can attach here? Perhaps #324

fungjj92 · 2017-12-22T18:19:25Z

src/angular/planit/src/app/risk-wizard/steps/hazard-step.component.html

-<button type="button" previousStep>Back</button>
+<div class="step-header">
+  <h3>Hazard</h3>
+  <h5>{{ risk.hazard }}</h5>


This is blank because the controller doesn't have a this.risk. I was having trouble myself accessing data from other wizard steps. It is available via this.session._data.KEY but exposing a protected object is not ideal. I have yet to play with a getKeyData() on wizard-session.service but I am skeptical that it would work.

Issue here was actually a miss from changes in #322 that tweaked the Risk model. This binding should be risk.weatherEvent?.name. I'll update.

Not sure what you mean by trouble accessing the risk data, perhaps we could pair on that briefly to better identify the problem you're having, as that seems separate from this PR.

Oh i guess i missed that too hah.

fungjj92 · 2017-12-22T18:27:38Z

src/angular/planit/src/app/risk-wizard/steps/hazard-step.component.ts

+
+  public directionalOptions = OrgRiskDirectionalOptions;
+  public relativeOptions = OrgRiskRelativeOptions;
+  // Can't *ngFor a map type or iterable, so instead we realize the iterable and use that in *ngFors


stanks but useful comment

fungjj92 · 2017-12-22T18:30:48Z

src/angular/planit/src/app/risk-wizard/steps/hazard-step.component.ts

+import { WizardStepComponent } from '../wizard-step.component';
+import { WizardSessionService } from '../wizard-session.service';
+
+interface HazardStepFormModel {


put in it's own file? the identify step form model is in it's own file. It's exceptionally unlikely that this model will be used anywhere else independently so fundamentally I'm not opposed to this being here, but we should be consistent either way.

Good catch and good point. Given that this is for now a private interface on both components, I'll probably move the one in identify back to the component and remove the exports on these.

fungjj92 · 2017-12-22T18:33:59Z

src/angular/planit/src/app/risk-wizard/steps/identify-step.component.html

+  <h3>Identify risk</h3>
+</div>
+<div class="step-content">
+  <p>Match a hazard to a subset of people, strucutres or assets (sometimes referred to as a "community system") that the selected hazard may impact</p>


typo: structures

fungjj92 · 2017-12-22T18:36:20Z

src/angular/planit/src/app/risk-wizard/steps/identify-step.component.html

+    </div>
+  </form>
+</div>
+<div class="step-footer">
  <button type="button" [disabled]="!form.valid" nextStep (finalize)="save()">Continue</button>


i think the buttons are generally considered part of the <form>

Generally yes, if they directly interact with form events, like a form submit button. That's not the case here though, these buttons are a part of wizard navigation not the form itself.

fungjj92 · 2017-12-22T18:51:59Z

src/angular/planit/src/app/risk-wizard/steps/hazard-step.component.html

+    </div>
+    <div class="step-form-control">
+      <h5>Intensity <span class="icon icon-question-circle-o" tooltip="{{ tooltipText.intensity }}"></span></h5>
+      <div class="btn-group">


I think buttons are what we're looking for rather than btn-group. The behavior is off too. If I change my selection, the buttons don't always respond. It would be nice to be able to select "unsure" again, and that is not possible. I'm on Chrome.

Whoa. Weird. The actual issue is that you can't select a button to the left of one you've already selected. That's new. Will fix.

I don't think the implementation of button vs button group really matters aside from button group is more semantically correct (since it enforces only one selected at a time) and we can separate the buttons with CSS. I'll play with it either way.

Well look what I found: valor-software/ngx-bootstrap#2581

Guess I'll switch to a standard button implementation real quick.

Didn't address button spacing. That's fairly straightforward and can be addressed later in a styling pass.

CloudNiner · 2017-12-22T20:39:02Z

@fungjj92 ready for another look.

fungjj92

Whoot!

The ngx-bootstrap button group btnRadio directive is busted for reactive forms, so we just implement directly with events for now instead: valor-software/ngx-bootstrap#2581

In shared module

If form is untouched

WIP: Add HazardStepComponent

e863f7f

Adds and use models for HazardStep form controls: - OrgRiskRelativeOption - OrgRiskDirectionalOption

CloudNiner assigned fungjj92 Dec 22, 2017

CloudNiner requested a review from fungjj92 December 22, 2017 16:45

CloudNiner added the in review label Dec 22, 2017

fungjj92 reviewed Dec 22, 2017

View reviewed changes

fungjj92 approved these changes Dec 22, 2017

View reviewed changes

CloudNiner added 9 commits December 22, 2017 15:53

Directly update freq/intensity form controls

e71a31e

The ngx-bootstrap button group btnRadio directive is busted for reactive forms, so we just implement directly with events for now instead: valor-software/ngx-bootstrap#2581

Fixup wizard init based on changes in #367

5147a6f

Move probability dropdown to new OptionDropdown

e2a7c46

In shared module

Move sass files for shared components

dee5299

Add HazardStepComponent tooltips

d06641f

Disable HazardStepComponent continue button

2d52e69

If form is untouched

Use branding colors for various archwizard states

0b8aab5

Give wizard steps consistent styling

336e1b7

Code review cleanup

70cf0a8

CloudNiner force-pushed the feature/awf/va-risk-wizard-step-two#236 branch from 75f13a1 to 70cf0a8 Compare December 22, 2017 20:54

CloudNiner merged commit 47c76f3 into develop Dec 22, 2017

hectcastro removed the in review label Dec 22, 2017

CloudNiner deleted the feature/awf/va-risk-wizard-step-two#236 branch December 22, 2017 20:57

nanotubing unassigned fungjj92 Apr 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VA Risk Wizard step two #370

VA Risk Wizard step two #370

CloudNiner commented Dec 22, 2017

fungjj92 left a comment

fungjj92 Dec 22, 2017

fungjj92 Dec 22, 2017

CloudNiner Dec 22, 2017

fungjj92 Dec 22, 2017

fungjj92 Dec 22, 2017

fungjj92 Dec 22, 2017

CloudNiner Dec 22, 2017

fungjj92 Dec 22, 2017

fungjj92 Dec 22, 2017

CloudNiner Dec 22, 2017

fungjj92 Dec 22, 2017

CloudNiner Dec 22, 2017

CloudNiner Dec 22, 2017

CloudNiner Dec 22, 2017

CloudNiner commented Dec 22, 2017

fungjj92 left a comment

		@@ -29,8 +29,12 @@ export class RiskWizardComponent implements AfterViewInit, OnDestroy, OnInit {

		ngOnInit() {
		// TODO: Set initial risk from API

VA Risk Wizard step two #370

VA Risk Wizard step two #370

Conversation

CloudNiner commented Dec 22, 2017

Overview

Demo

Blank form for new risk (continue disabled)

OptionDropdown open to select probability

After selecting other items (continue enabled once one form control changed)

Continue to next step (selections printed to console.log on navigation)

Back to previous step (form selections saved)

Notes

Testing Instructions

fungjj92 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

CloudNiner commented Dec 22, 2017

fungjj92 left a comment

Choose a reason for hiding this comment