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.

Sign up for GitHub

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

Jump to bottom

feat(config): Support for custom error component & positioning #15

Merged
merged 12 commits into from
Jul 10, 2020
Merged

feat(config): Support for custom error component & positioning #15

merged 12 commits into from
Jul 10, 2020

Conversation

harikvpy
Copy link
Contributor

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
[x] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #14

What is the new behavior?

Introduces two new global config options -- controlErrorComponent & controlErrorComponentAnchorFn. Both are documented.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@harikvpy
Copy link
Contributor Author

I have updated the sample app to show how the two new configuration settings can be used. While I'm somewhat sure about the correctness of the controlErrorComponent implementation, I can't share the same positivity about controlErrorComponentAnchorFn.

My sample shows repositioning the error component using DOM manipulation, which I'm not so certain is the Angular way of doing things. I resorted to this approach as I couldn't figure out an Angular API to do it cleanly, ie, using their well defined interfaces. Perhaps there's one. If there's, please share. Tks.

NetanelBasal
NetanelBasal requested changes Jun 26, 2020
View reviewed changes
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
projects/ngneat/error-tailor/src/lib/control-error.component.ts Outdated Show resolved Hide resolved
projects/ngneat/error-tailor/src/lib/control-error.component.ts Outdated Show resolved Hide resolved
projects/ngneat/error-tailor/src/lib/providers.ts Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@harikvpy
Copy link
Contributor Author

I have completed the changes that you requested along with the necessary updates to the README. I also created a sample Ionic project which is here. It currently statically links the amended source, but once the PR is merged, I'll change that to use the npm package. If you want to merge or refer to this sample in the README, pls let me know. Thanks.

NetanelBasal
NetanelBasal reviewed Jun 29, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
NetanelBasal
NetanelBasal reviewed Jun 29, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
NetanelBasal
NetanelBasal reviewed Jun 29, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
NetanelBasal
NetanelBasal reviewed Jun 29, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
NetanelBasal
NetanelBasal requested changes Jun 29, 2020
View reviewed changes
Copy link
Member

@NetanelBasal NetanelBasal left a comment

Choose a reason for hiding this comment

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

Great changes.

@harikvpy
Copy link
Contributor Author

harikvpy commented Jul 6, 2020

Made all the changes as per your suggestions except for a test for the destroy function (see reply to your comment). Let me know if I missed out something. Thanks again an apologies for the slight delay.

NetanelBasal
NetanelBasal reviewed Jul 7, 2020
View reviewed changes
Copy link
Member

@NetanelBasal NetanelBasal left a comment

Choose a reason for hiding this comment

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

LGTM. I'll play with the code later and keep you update.

src/app/app.module.ts Outdated
@@ -23,7 +39,9 @@ import { ErrorTailorModule } from '@ngneat/error-tailor';
};
},
deps: []
}
},
controlErrorComponent: CustomControlErrorComponent, // Uncomment to see errors being rendered using a custom component
Copy link
Member

Choose a reason for hiding this comment

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

Uncomment again :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I remove the whole custom control specification from the sample app? Sample code for this in the README and I believe is well captured there. So may be it's not necessary?

Copy link
Member

Choose a reason for hiding this comment

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

No, just put it in a comment. It's good for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@NetanelBasal
Copy link
Member

Resolve the conflicts, please.

NetanelBasal
NetanelBasal reviewed Jul 8, 2020
View reviewed changes
README.md Outdated
// Custom error component that will replace the standard DefaultControlErrorComponent.
@Component({
template: `
<ion-item lines="none" class="ion-text-wrap" [class.hide-control]="hide">
Copy link
Member

Choose a reason for hiding this comment

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

Let's make the public properties more readable for the consumers:

hide => hideError
_tpl => errorTemplate
_text => errorText
context => errorContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent idea. Small things like this can go a long way to ease the pain of using a library!

NetanelBasal
NetanelBasal reviewed Jul 8, 2020
View reviewed changes
src/app/custom-error-control/custom-error-control.component.ts Outdated
]
})
export class CustomControlErrorComponent extends DefaultControlErrorComponent {
constructor(cdr: ChangeDetectorRef, host: ElementRef<HTMLElement>) {
Copy link
Member

Choose a reason for hiding this comment

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

Omit the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

NetanelBasal
NetanelBasal reviewed Jul 9, 2020
View reviewed changes
README.md Outdated
export class AppModule {}
```

### Per Control Config
Copy link
Member

Choose a reason for hiding this comment

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

Remove this section. It's in the inputs section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I also removed the redundant Global Config subsection.

NetanelBasal
NetanelBasal reviewed Jul 9, 2020
View reviewed changes
README.md Outdated
@@ -210,6 +210,7 @@ The library adds a `form-submitted` to the submitted form. You can use it to sty

## Config

### Global Config
Copy link
Member

Choose a reason for hiding this comment

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

remove ### Global Config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

NetanelBasal
NetanelBasal approved these changes Jul 9, 2020
View reviewed changes
Copy link
Member

@NetanelBasal NetanelBasal left a comment

Choose a reason for hiding this comment

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

Add yourself to the contributors' list by running npm run contributors:add

@NetanelBasal NetanelBasal merged commit c9d7776 into ngneat:master Jul 10, 2020
@harikvpy harikvpy mentioned this pull request Jul 14, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NetanelBasal NetanelBasal NetanelBasal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@harikvpy @NetanelBasal