Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logic to allow warnings for input fields #3256

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Exitare
Copy link
Member

@Exitare Exitare commented Jan 18, 2025

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.

image

Feedback is welcome

@FrancescoBorzi
Copy link
Collaborator

Build failing

@Exitare
Copy link
Member Author

Exitare commented Jan 18, 2025

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

@Helias
Copy link
Member

Helias commented Jan 19, 2025

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

This is the issue reported by the pipeline

@Exitare
Copy link
Member Author

Exitare commented Jan 19, 2025

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
Copy link
Member

Helias commented Jan 19, 2025

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

@Exitare
Copy link
Member Author

Exitare commented Jan 19, 2025

Thank you :)

@Exitare
Copy link
Member Author

Exitare commented Jan 19, 2025

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
Copy link
Member

Helias commented Jan 19, 2025

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
Copy link
Member Author

Exitare commented Jan 19, 2025

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 :)

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

Successfully merging this pull request may close these issues.

[Enchantement] - Mandotory fields colour/warning.
3 participants