Add logic to allow warnings for input fields

[Exitare]

Does address but does not close #3236.
Tests are failing, because I dont want to fix them before this idea is either accepted or denied.
Right now, this is only working for the creature template max level field.

Feedback is welcome

[FrancescoBorzi]

Build failing

[Exitare]

@FrancescoBorzi Works on my end. Can you please provide more information ?

[Helias]

We have a lint rule that notifies if a component is not using the OnPush strategy, it's complaining about it

This is the issue reported by the pipeline

[Exitare]

Thank you, but the failed test is intentional.
I would love to hear some feedback on my proposed solution before I fix any tests that will break due to the change. If the solution is rejected I didn't waste too much time :)
Hope that makes sense!

[Helias]

ok, I will test this PR locally and let you know ;-)

[Exitare]

Thank you :)

[Exitare]

I also would like to mention, that this will NOT address any editors. The example I added for the maxLevel in creature template is just for showcasing purposes of the proposed solution. The PR aims to provide an easy framework to add form validation, not to add form validation everywhere it is needed. This needs to happen gradually, otherwise this will be a gigantic PR, considering Item Template and Creature Template have to change along with a lot of other forms.

[Helias]

I don't dislike the approach but I am wondering if can be done only with:

              <input
                keiraInputValidation
                [formControlName]="'maxlevel'"
                id="maxlevel"
                type="number"
                class="form-control form-control-sm"
              />

Without requiring any other information or adding components, because may you can find a way to get the formcontrol from the input and you could spawn dynamically a component next to the without already writing a placeholder component <keira-validation-feedback>.
If this is not feasible without using the <keira-validation-feedback> component and the input parameter [keiraInputValidation]="editorService.form.get('maxlevel')" is mandatory, then I can approve this approach.

Thanks by the way for implementing this proof of concept of warnings, it's a good job!

[Exitare]

Thank you for providing feedback. I will try to make it work. I also had a solution in mind that minimizes the changes required to all editors as this can introduce quite the overhead in work. I will explore possible solutions and hopefully something will work :)

[Exitare]

In theory I could do something like this:

import { Directive, ViewContainerRef } from '@angular/core';

@Directive({
  selector: '[dynamicComponentHost]',
})
export class DynamicComponentHostDirective {
  constructor(public viewContainerRef: ViewContainerRef) {}
}

import { Component, ComponentFactoryResolver, OnInit } from '@angular/core';
import { KeiraValidationFeedbackComponent } from './keira-validation-feedback.component';
import { DynamicComponentHostDirective } from './dynamic-component-host.directive';

@Component({
  selector: 'app-dynamic-form',
  template: `
    <div class="form-group col-12 col-sm-6 col-md-4 col-lg-3 col-xl-2">
      <label for="maxLevel">Max Level</label>
      <input
        [keiraInputValidation]="editorService.form.get('maxlevel')"
        [formControlName]="'maxlevel'"
        id="maxlevel"
        type="number"
        class="form-control form-control-sm"
        (blur)="showValidationFeedback()"
      />
      <ng-template dynamicComponentHost></ng-template>
    </div>
  `,
})
export class DynamicFormComponent implements OnInit {
  @ViewChild(DynamicComponentHostDirective, { static: true })
  dynamicHost!: DynamicComponentHostDirective;

  constructor(
    private resolver: ComponentFactoryResolver,
    public editorService: EditorService
  ) {}

  ngOnInit(): void {}

  showValidationFeedback(): void {
    const control = this.editorService.form.get('maxlevel');
    if (control?.invalid && control.touched) {
      const viewContainerRef = this.dynamicHost.viewContainerRef;
      viewContainerRef.clear(); // Clear any existing components
      const componentFactory =
        this.resolver.resolveComponentFactory(KeiraValidationFeedbackComponent);
      const componentRef = viewContainerRef.createComponent(componentFactory);
      componentRef.instance.control = control; // Pass the control to the component
    }
  }
}

Used like this:

<div class="form-group col-12 col-sm-6 col-md-4 col-lg-3 col-xl-2">
  <label for="maxLevel">Max Level</label>
  <input
    [keiraInputValidation]="editorService.form.get('maxlevel')"
    [formControlName]="'maxlevel'"
    id="maxlevel"
    type="number"
    class="form-control form-control-sm"
    (blur)="showValidationFeedback()"
  />
  <ng-template dynamicComponentHost></ng-template>
</div>

But personally, I think this is a lot of boilerplate code and super convoluted too, for a worse outcome in terms of readability and most likely maintainability too.

If you have a better solution or something I should check out, please let me know :)

[Helias]

I was convinced that everything can be implemented inside the directive attribute making it agnostic and generic for all inputs without requiring any input information.

import { Directive, ElementRef, inject, OnInit, Renderer2 } from '@angular/core';
import { AbstractControl, NgControl } from '@angular/forms';
import { SubscriptionHandler } from '@keira/shared/utils';

@Directive({
  selector: '[keiraInputValidation]',
  standalone: true,
})
export class InputValidationDirective extends SubscriptionHandler implements OnInit {
  private readonly el: ElementRef = inject(ElementRef);
  private readonly renderer: Renderer2 = inject(Renderer2);
  private readonly ngControl: NgControl = inject(NgControl);

  private errorDiv: HTMLElement | null = null;

  ngOnInit(): void {
    const control = this.ngControl.control;

    if (!control) {
      return;
    }

    this.subscriptions.push(
      control.statusChanges?.subscribe(() => {
        this.updateErrorMessage(control);
      }),
    );
  }

  private updateErrorMessage(control: AbstractControl): void {
    if (this.errorDiv) {
      this.renderer.removeChild(this.el.nativeElement.parentNode, this.errorDiv);
      this.errorDiv = null;
    }

    if (control?.touched && control?.invalid) {
      const errorMessage = control?.errors?.['required'] ? 'This field is required' : 'Invalid field';

      this.errorDiv = this.renderer.createElement('div');

      const text = this.renderer.createText(errorMessage);
      this.renderer.appendChild(this.errorDiv, text);

      const parent = this.el.nativeElement.parentNode;
      this.renderer.appendChild(parent, this.errorDiv);
    }
  }
}

<div class="form-group col-12 col-sm-6 col-md-4 col-lg-3 col-xl-2">
  <label for="maxLevel">Max Level</label>
  <input keiraInputValidation [formControlName]="'maxlevel'" id="maxlevel" type="number" class="form-control form-control-sm" />
</div>

This will allow us to add the directive keiraInputValidation to any input without further boilerplate or implementation, moreover, now the directive will be self-contained and it does not require any other helper component like the keira3-validation-feedback.
I am not a fan of the Renderer2 service but if we need to manipulate the DOM it's the solution for Angular.

You can find the commit here 7021a3a
The branch name is warnings-directive

EDIT: this code is a draft based on your code, you can re-use it improving it if possible, we could continue the implementation in this branch/PR

[Exitare]

@Helias
Wow! thank you. I wasn't aware of this possibility. That looks great. I will copy it over :)

[Exitare]

@Helias
I added the directive and it works, with a twist.
If the creature template is opened up the first time one can clear the field and put another field in focus (e.g. click on minlevel), the error message will not show up. One has to enter a value again and then delete it, to actually trigger the error message.
I will look into this.

Additionally, if we add the validation, we are just one step shy of adding a subject that the keira edit button can subscribe to, to disable them if validation fails.
What are your thoughts on that?

[Helias]

I noticed that, if we remove the control.touched contidion it should work on the first time too, I am not sure if it's 100% correct but we could remove it and then think if we really need that condition or not.

About the validation, that's a good idea actually 🚀

[Exitare]

I added a workaround, by adding markedAsTouched() when the field is invalid. But maybe thats not as good as a solution as it can be?

[Helias]

I added a workaround, by adding markedAsTocuched() when the field is invalid. But maybe thats not as good as a solution as it can be?

it could be in this case, let's keep it

[Helias]

not sure about this code inside the else block, it seems more a debug code , we could keep it, but could you give me more insight about it?

[Francesco-Borzi]

@Exitare please do not mark comments as resolved without answering or addressing them

[Exitare]

I resolved it, because I removed it initially. But then one of the tests failed and I added it back, but didn't unresolve the issue here.

[Helias]

I think that I wrote this, but I am not sure if we have a class error-message, if not, we could remove this line of code

[Exitare]

We dont. But the tests use it to get the div in order to verify the correct message.

[Helias]

I would use an id instead of a class for this

[Francesco-Borzi]

why does it have to be a singleton?

[Exitare]

Because validation should happen on a page by page basis and not on the app level ?

[Helias]

It should not be necessary, we could keep one instance for the entire application, so provide it in "root" and don't put it here.

If you find a strong reason to provide it inside the component then we could put it back.

[Francesco-Borzi]

Suggested change

	const defaultValue = defaultValues[field];
	this._form.addControl(field, new FormControl(defaultValue, [Validators.required]));
	this._form.addControl(field, new FormControl(defaultValues[field], [Validators.required]));

[Francesco-Borzi]

I don't think we need this comment, IMHO the code seems already quite self-explanatory :)

Suggested change

// Loop through the fields and initialize controls with default values

[Francesco-Borzi]

is this change needed for the original purpose of the PR? in general, I'd prefer to split changes in several PRs, for example:

• 1 PR to include new functionality (including its unit tests)
• a separate PR to improve/refactor existing code that is not related to the functionality being introduced

[Francesco-Borzi]

I think we should avoid this repetition and wrap validationService.validationPassed$ in a variable. Even better to use a Signal.

[Exitare]

I don't know how this requested change should look like because I used a variable before and it was requested to use the async pipe. Now it's back to variable again?

[Helias]

Before it was a variable with a subscription running on ngOninit, this affects the component adding a dependency for the OnInit interface and moreover, it forces you to manage the subscribe/unsubscribe.
In the current situation you don't need ngOninit and to manage the subscribe/unsubscribe thanks to the async pipe.

Wrapping this into a variable can be done without ngOninit still and in the following way:

protected readonly isValidationPassed$ =  this.validationService.validationPassed$.pipe(map((val) => val === false));

Keeping in the template

[disabled]="isValidationPassed$ | async"

Or, you could use signals

[Exitare]

OK, for learning purposes. What's the undesirable part about having a dependency in the ngOnInit, besides the subscription management part. Is it a general best practice not to use the ngOnInit if possible?

[Helias]

Personally I avoid any kind of dependencies, so if we can avoid ngOnInit, ngOnChanges etc. in an elegant way I would not add them because It increases the complexity of the logic component

[Francesco-Borzi]

I think it's better to create a lib for validation instead of creating one lib for "directives" that can contain several directives that are unrelated to them.

So in the validation lib you can have all generic tools that are being used for the validation, including the validation service.

[Exitare]

Interesting approach. I placed the files in the respective libs as this is done with several other functionalities too. There is no lib sql service either and so on. They are all pretty generic.