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

The Bar Rating Component always has the Validator required:true, even if we do not set it to be required, due to component logic #116

Closed
1 task done
ben-pearson-korelabs-co opened this issue Sep 13, 2024 · 5 comments · Fixed by #123
Labels

Comments

@ben-pearson-korelabs-co

I am submitting a bug

  • Bug Report

What is the expected behavior?

When we set in our form control for the control to not be required, then the bar rating component should not have a required:true error.

What is the current behavior?

When we do not add the required directive to the bar-rating, it still acts as required, because if the bar-rating value is 0, there is some logic in the component that sets the errors: { required: true }

What are the steps to reproduce?

Link to stackblitz: https://stackblitz.com/edit/ngx-bar-rating-er6r7z?file=src%2Fmain.ts,src%2Findex.html

What is the use-case or motivation for changing an existing behavior?

The component is always required and it should not be.

Which versions are you using for the following packages?

7.0.1

Angular:
ngx-bar-rating:

Is there anything else we should know?

I think it might be this validate() method? If the bar rating value is !0, then it will set required:true... But we should be able to have 0 as the rating is not required.

validate(c: UntypedFormControl): { required: boolean } | null {
    return (this.required && !c.value) ? { required: true } : null;
  }
@MurhafSousli
Copy link
Owner

So you mean we should use c.value != null instead?

@ben-pearson-korelabs-co
Copy link
Author

Hello, apologies for the late reply!

Yes I believe that would resolve the issue.

I also noticed that this.required would always evaluate to true as it's defined as a function rather than a simple boolean variable but I'll leave that with you :)

Thanks!

Screenshot 2024-10-01 104619

@MurhafSousli
Copy link
Owner

After investigating, the logic we currently have is correct. we should NOT have 0 as a valid rating. because the minimum rating is 1 star right? you cannot rate a place on google maps with 0 star, even if we wish. 1 star is minimum input. 0 means user didn't rate. so if the control is required and 0 is the rating it should display an error.

@ben-pearson-korelabs-co
Copy link
Author

After investigating, the logic we currently have is correct. we should NOT have 0 as a valid rating. because the minimum rating is 1 star right? you cannot rate a place on google maps with 0 star, even if we wish. 1 star is minimum input. 0 means user didn't rate. so if the control is required and 0 is the rating it should display an error.

Absolutely 0 can be an invalid rating and definitely if the control is set to required and the value is zero, it should error. The problem I am having is that when the control is set to Not required (I do not pass Validators.required in the formControl() method on formGroup initialisation) and the value is 0, it still sets the required attribute of the control to true.

My previous stack blitz might have been confusing so I have updated: https://stackblitz.com/edit/ngx-bar-rating-hvzuexue?file=src%2Fmain.ts,src%2Findex.html

In my use case, this form control is part of a form group, and the user doesn't have to provide a rating, but the form group is never valid because the rating required property is always true.

@MurhafSousli
Copy link
Owner

Yea, this is a bug, I forgot to use the required as a signal after converting the inputs to signal inputs.

return (this.required && !c.value) ? { required: true } : null;

@MurhafSousli MurhafSousli linked a pull request Jan 16, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants