NAS-132300 / 25.04 / Create a configurable full-page form with glossary section and help #11091

RehanY147 · 2024-11-22T23:22:13Z

Changes:

Introduces configurable full-page form with glossary section and help

Testing:

Test the containers wizard. It should work correctly

bugclerk · 2024-11-22T23:22:26Z

Jira URL: https://ixsystems.atlassian.net/browse/NAS-132300

codecov · 2024-11-22T23:54:13Z

Codecov Report

Attention: Patch coverage is 23.71134% with 148 lines in your changes missing coverage. Please review.

Project coverage is 82.19%. Comparing base (8f8ac70) to head (b8c7526).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...s/ix-full-page-form/ix-full-page-form.component.ts	0.00%	62 Missing ⚠️
...m-with-glossary/ix-form-with-glossary.component.ts	0.00%	47 Missing ⚠️
...modules/forms/ix-forms/services/ix-form.service.ts	4.54%	21 Missing ⚠️
...rms/ix-forms/directives/with-glossary.directive.ts	0.00%	11 Missing ⚠️
...orm-section/ix-full-page-form-section.component.ts	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11091      +/-   ##
==========================================
- Coverage   82.39%   82.19%   -0.20%     
==========================================
  Files        1648     1653       +5     
  Lines       57884    58082     +198     
  Branches     5955     6002      +47     
==========================================
+ Hits        47691    47738      +47     
- Misses      10193    10344     +151

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

undsoft

Also Save button is not clickable for me when I fill in the form. (I don't see any validation errors).

undsoft · 2024-11-26T11:10:22Z

src/app/modules/forms/ix-forms/components/ix-full-page-form/ix-full-page-form.component.ts

+  searchControl = this.formBuilder.control('');
+  searchOptions = signal<Option[]>([]);
+  onSubmit = output();
+  buttonText = input.required<string>();


This part is a bit hard to read. Let's group inputs and outputs and add protected/private for other fields.

undsoft · 2024-11-26T11:11:38Z

src/app/modules/forms/ix-forms/components/ix-full-page-form/ix-full-page-form.component.ts

+  searchMap = input<Map<string, string>>();
+  pageTitle = input.required<string>();
+  isLoading = input.required<boolean>();
+  subscription = new Subscription();


undsoft · 2024-11-26T11:12:02Z

src/app/modules/forms/ix-forms/components/ix-full-page-form/ix-full-page-form.component.ts

+  formGroup = input.required<FormGroup>();
+  requiredRoles = input<Role[]>();
+  searchMap = input<Map<string, string>>();
+  pageTitle = input.required<string>();


I think this component is trying to have too much responsibility. I don't think it needs to take care of page header or save button. It could just be a component responsible for form glossary that can be included on other pages.

undsoft · 2024-11-26T11:22:26Z

src/app/pages/virtualization/components/instance-wizard/instance-wizard.component.ts

@@ -89,6 +96,19 @@ export class InstanceWizardComponent implements OnInit {
    }))),
  );

+  protected searchMap = new Map<string, string>([


For this pull master and see ixRegisteredControl and NavigateAndInteractService. We could extend the former to also include control labels.

undsoft · 2024-11-26T11:25:27Z

src/app/pages/virtualization/components/instance-wizard/instance-wizard.component.ts

+    }),
+  ));
+
+  protected isEnvironmentValid = toSignal(this.form.controls.environmentVariables.valueChanges.pipe(


Would be good to come with a way to avoid this. Maybe you could come up with a way for a section to figure out which fields are rendered underneath it via contentChildren or some sort of injection.

undsoft · 2024-12-04T08:36:51Z

src/app/modules/forms/ix-forms/components/ix-button-group/ix-button-group.component.ts

@@ -41,6 +41,8 @@ export class IxButtonGroupComponent implements ControlValueAccessor {
  @HostBinding('class.inlineFields')
  @Input() inlineFields = false;

+  formControlName = input<string>();


Did you want to use this.controlDirective.name or label input?

undsoft · 2024-12-04T08:42:00Z

src/app/modules/forms/ix-forms/components/ix-full-page-form/ix-full-page-form.component.ts

+
+@UntilDestroy()
+@Component({
+  selector: 'ix-full-page-form',


If this is not used, it needs to be removed.

undsoft · 2024-12-04T08:44:10Z

...p/modules/forms/ix-forms/components/ix-form-with-glossary/ix-form-with-glossary.component.ts

+    this.handleSearchControl();
+  }
+
+  protected onSectionClick(id: string, label: string = null): void {


You can update NavigateAndInteractService to have a separate method for navigating to something by id and move this code there.

undsoft · 2024-12-04T08:48:04Z

src/app/modules/forms/ix-forms/services/ix-form.service.ts

@@ -13,20 +17,79 @@ export class IxFormService {
    return this.getControls().map((ctrl) => ctrl.name);
  }

+  private getControlsOptions(query = ''): Option[] {


I don't think IxFormService should care about searching within set of controls. Seems like a job for the component where it's needed.

What is elementsWithIds?

The code in two blocks is similar.

undsoft · 2024-12-04T08:49:12Z

src/app/modules/forms/ix-forms/services/ix-form.service.ts

  }

  registerControl(control: NgControl, elementRef: ElementRef<HTMLElement>): void {
    this.controls.set(control, elementRef.nativeElement);
+    this.controlsOptions.set(this.getControlsOptions());
  }

  unregisterControl(control: NgControl): void {


This is unused now, but it was used some time before. We should have ngOnDestroy in registeredControl and call this.

undsoft · 2024-12-04T08:51:20Z

src/app/modules/forms/ix-forms/directives/with-glossary.directive.ts

+import { IxFormWithGlossaryComponent } from 'app/modules/forms/ix-forms/components/ix-form-with-glossary/ix-form-with-glossary.component';
+
+@Directive({
+  selector: '[withGlossary]',


What's the advantage of making this a directive?
Could we not make it an ordinary component instead and let people place glossary in their forms themselve?

undsoft · 2024-12-04T08:55:41Z

src/app/modules/forms/ix-forms/components/ix-button-group/ix-button-group.component.ts

@@ -51,6 +53,10 @@ export class IxButtonGroupComponent implements ControlValueAccessor {
    this.controlDirective.valueAccessor = this;
  }

+  @HostBinding('attr.id') get id(): string {
+    return this.formControlName() || '';


Also you set id here, but also set ix-label in ixRegisteredControl. Can you use one of these?

In some cases, label might not be set. In other cases, id might not be set. So, these give more of a chance that at least one will be set and allow usage for the ixRegisteredControl directive.

undsoft · 2024-12-09T12:31:16Z

src/app/directives/navigate-and-interact/navigate-and-interact.service.ts

@@ -22,7 +22,7 @@ export class NavigateAndInteractService {
    });
  }

-  private handleHashScrollIntoView(htmlElement: HTMLElement): void {
+  handleHashScrollIntoView(htmlElement: HTMLElement): void {


Method needs to be renamed if it's going to be public.

undsoft · 2024-12-09T12:33:19Z

src/app/modules/forms/ix-forms/services/ix-form.service.ts

-  private controls = new Map<NgControl, HTMLElement>();
+  private controls = new Map<string, HTMLElement>();
+  private sections = new Map<IxFormSectionComponent, Map<string, NgControl | null>>();
+  controlNamesWithlabels$ = new BehaviorSubject<ControlNameWithLabel[]>([]);


controlNamesWithLabels

undsoft · 2024-12-09T12:49:11Z

src/app/pages/virtualization/components/instance-wizard/instance-wizard.component.html

        [required]="true"
      ></ix-input>

      <ix-checkbox
+        ixRegisteredControl


The original intent was to have this directive in ix form controls and not to expose it in other places. Can't we keep doing that instead of having registeredControl here?

RehanY147 added 4 commits November 22, 2024 22:31

NAS-132300: Add id to ix-forms components

0a011be

NAS-132300: Created full-page-form

01f9e38

NAS-132300: Replaced instance-wizard

f19660d

NAS-132300: Update instance-wizard.component.html

b9c6fba

bugclerk changed the title ~~Create a configurable full-page form with glossary section and help~~ NAS-132300 / 25.04 / Create a configurable full-page form with glossary section and help Nov 22, 2024

undsoft self-requested a review November 25, 2024 15:52

undsoft requested changes Nov 26, 2024

View reviewed changes

RehanY147 added 6 commits November 30, 2024 05:14

NAS-132300: Better separation of properties

b67d462

NAS-132300: Update ix-full-page-form.component.ts

3ae6564

Merge branch 'master' into NAS-132300

d4df88c

NAS-132300: Added new components

50240cf

NAS-132300: Clean up instance-form

b8c7526

NAS-132300: Using directive approach

a08540d

RehanY147 requested a review from undsoft December 4, 2024 01:13

undsoft reviewed Dec 4, 2024

View reviewed changes

RehanY147 added 10 commits December 5, 2024 05:38

NAS-132300: Remove form-section and use controlDirective for names

9788e13

NAS-132300: Revert required labels

bdb11ba

NAS-132300: Clean up non-control registration

cbca47a

NAS-132300: Remove attr.id

540f2cb

NAS-132300: Add section labels and validity info

282538a

NAS-132300: Create ix-form-glossary.component.ts

7392d5c

NAS-132300: Update ix-form-glossary.component.ts

9c64de1

NAS-132300: Update ix-form-glossary.component.ts

6c2278a

NAS-132300: Update ix-form-glossary.component.ts

c578de1

NAS-132300: Add section click

50592df

undsoft self-requested a review December 7, 2024 08:05

undsoft reviewed Dec 9, 2024

View reviewed changes

RehanY147 added 3 commits December 10, 2024 14:21

NAS-132300: Renamed method

3b8a2f8

NAS-132300: Renamed controlNamesWithLabels

a4b2c11

NAS-132300: Automatic directive application

70913bd

RehanY147 marked this pull request as ready for review December 12, 2024 05:36

RehanY147 requested a review from a team as a code owner December 12, 2024 05:36

RehanY147 requested review from denysbutenko and removed request for a team December 12, 2024 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NAS-132300 / 25.04 / Create a configurable full-page form with glossary section and help #11091

NAS-132300 / 25.04 / Create a configurable full-page form with glossary section and help #11091

RehanY147 commented Nov 22, 2024

bugclerk commented Nov 22, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

undsoft left a comment

undsoft Nov 26, 2024

undsoft Nov 26, 2024

undsoft Nov 26, 2024

undsoft Nov 26, 2024

undsoft Nov 26, 2024

undsoft Dec 4, 2024

undsoft Dec 4, 2024

undsoft Dec 4, 2024

undsoft Dec 4, 2024

undsoft Dec 4, 2024

undsoft Dec 4, 2024

undsoft Dec 4, 2024

RehanY147 Dec 7, 2024

undsoft Dec 9, 2024

undsoft Dec 9, 2024

undsoft Dec 9, 2024

NAS-132300 / 25.04 / Create a configurable full-page form with glossary section and help #11091

Are you sure you want to change the base?

NAS-132300 / 25.04 / Create a configurable full-page form with glossary section and help #11091

Conversation

RehanY147 commented Nov 22, 2024

bugclerk commented Nov 22, 2024

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

undsoft 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2024 •

edited

Loading