Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

feat: rich error message POC #4

Closed
wants to merge 1 commit into from
Closed

Conversation

trunkovich
Copy link

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?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #3

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@trunkovich
Copy link
Author

@DmitryEfimenko I've created PR to show your POC how #3 can be implemented. I didn't find a way to solve it without structural directive.

Also maybe we can do this syntax:

<div ngxErrors="zip">
  <div ngxError="required">Required</div>
  <div ngxError="minlength">
    <div *ngxRichErrorMessage="
                let actualLength = actualLength;
                let requiredLength = requiredLength">
        Value should be at least {{ requiredLength }} long; Right now it's
        {{ actualLength }}
    </div>
  </div>
</div>

@DmitryEfimenko
Copy link
Contributor

I like the idea a lot.

Perhaps there's no need for a separate directive. The original ngxError directive could expose this functionality.
With the change in place the syntax for the old use case would not change:

<div ngxErrors="zip">
  <div ngxError="minlength">Value should be at least 5 long</div>
</div>

Below is the syntax for accessing the error object. In addition, we could put the error object into the $implicit value for the object passed when creating an embedded view to simplify boilerplate.

<div ngxErrors="zip">
  <div *ngxError="'minlength'; let err">
    Value should be at least {{ err.requiredLength }} long; Right now it's {{ err.actualLength }}
  </div>
</div>

if (!this.ref) {
this.ref = this.viewContainer.createEmbeddedView(
this.template,
this._context
Copy link
Contributor

Choose a reason for hiding this comment

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

{ $implicit: this._context } to enable syntax I mentioned in the general comments

@trunkovich
Copy link
Author

trunkovich commented May 11, 2020

@DmitryEfimenko do we actually can make regular directive behave as both structural and regular? Can you provide any example? I can' find it

The only way I see - make it only structural.

<div ngxErrors="zip">
  <div *ngxError="'requred'">
  <div *ngxError="'minlength'; let err">
    Value should be at least {{ err.requiredLength }} long; Right now it's {{ err.actualLength }}
  </div>
</div>

BTW, it seems to me that it should be structural. It works same way as *ngIf do

@DmitryEfimenko
Copy link
Contributor

You're right. It needs to be a structural directive, which I'm OK with. It's a bigger breaking change, but since we're changing API already, might as well sneak this change in. Besides, the library isn't all that popular yet. This is the time to make it right.

@DmitryEfimenko
Copy link
Contributor

another option is to actually create a separate structural directive, but promote syntax similar to the following:

<div ngxErrors="zip">
  <div ngxError="requred">Required</div>
  <div ngxError="minlength" *errorData="let err">
    Value should be at least {{ err.requiredLength }} long; Right now it's {{ err.actualLength }}
  </div>
</div>

This way *errorData becomes kind of optional and it does not interfere with the original ngxError directive.

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

Successfully merging this pull request may close these issues.

None yet

2 participants