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

improvement(api): explicit show/hide errors #21

Conversation

mlc-mlapis
Copy link
Contributor

@mlc-mlapis mlc-mlapis commented Jul 22, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: CONTRIBUTING.md#commit
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

It's not possible to programmatically show/hide validation errors.

What is the new behavior?

New API added to programmatically show/hide validation errors.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@mlc-mlapis
Copy link
Contributor Author

mlc-mlapis commented Jul 22, 2020

@NetanelBasal I tried to add some tests to make sure that the new methods work as expected. Because I am not a standard user of the Spectator, I am not sure what would be the best way to check, that after invoking hideError() (line # 161 in control-error.directive.spec.ts), the correct label ( <label class="control-error hide-control"></label>) has been set to the class hide-control (because the error component is not destroyed in fact). I mean the exact <label> that is related to the tested and concrete <control-error> component.

PS: A general question, how does the logic exactly work with the syntax spectator.query(byText('required error'))? It's possible to say that it checks all the text content of the actual component template and if both words required and error are found (even separately in different places), then it's truthy?

I would certainly use a separate simple component, with only one <input> placed, to have a more simplified situation for selection, but I would prefer to have a bit more combined case.

README.md Outdated
@@ -197,6 +197,28 @@ One typical case when to use it is radio buttons in the same radio group where i
<input [controlErrorsOnBlur]="false" formControlName="name" />
```

## Methods

- `showError()` - Allows programmatic access to show a control error component, even when no other invoking event had been happened (like a blur or a submit). There should be a validation error on such an element, of course.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section is too long. Make it shorter, please.

@rafaelss95
Copy link

rafaelss95 commented Oct 26, 2020

@NetanelBasal I have an use case where I add error/validator manually and call updateValueAndValidity and no error message is rendered as there's no user interaction in this operation... so it would be great if this PR gets merged so that I could call showError manually.

Btw, here a demonstration: Stackblitz demo


Another use case that I have is to show errors in a stepper once the user click to proceed without filling all fields correctly (as the "Proceed" button in each stepper is type="button" the lib doesn't doesn't show errors automatically).

@NetanelBasal
Copy link
Member

This PR isn't ready.

@rafaelss95
Copy link

rafaelss95 commented Oct 26, 2020

This PR isn't ready.

Hmmm.. could I help somehow to speed this up or will @mlc-mlapis conclude this? I'm asking because without this is being really difficult to show errors for the cases abovementioned.

@NetanelBasal
Copy link
Member

Any news here?

@mlc-mlapis
Copy link
Contributor Author

@NetanelBasal, I apologize for that delay. I should prioritize it and hope that I would be able to do the necessary during this week.

@mlc-mlapis
Copy link
Contributor Author

@NetanelBasal I have finalized the required tests and hope that it looks fine for you now. 😃 Also, I have reduced descriptions in README.md and merged the actual master branch with my branch, of course. Please, look at it. Thanks.

@NetanelBasal NetanelBasal merged commit 864821b into ngneat:master Mar 2, 2021
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.

None yet

3 participants