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

refactor: improve form validation #962

Open
wants to merge 5 commits into
base: next
Choose a base branch
from
Open

refactor: improve form validation #962

wants to merge 5 commits into from

Conversation

Enes5519
Copy link
Contributor

customInvalidText, forceCustomError and clearCustomError are deprecated

add error attribute

customInvalidText, forceCustomError and clearCustomError are deprecated
@Enes5519 Enes5519 changed the title Improve Form Validation refactor: improve form validation Dec 9, 2024
@Enes5519 Enes5519 marked this pull request as ready for review December 9, 2024 13:08
get customInvalidText(): string {
return this._customInvalidText;
}

@property({ reflect: true, type: String })
error: string = "";
Copy link
Member

Choose a reason for hiding this comment

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

no need to give type, it's already inferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this decorator is trained in TypeScript. It doesn't hurt to have it as an extra, bro.

Copy link
Member

Choose a reason for hiding this comment

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

it's already inferred as part of TypeScript, no additional work required for decorator. since we don't have explicit types for other values it's better to remove it

*/
async forceCustomError() {
await this.updateComplete;
this.validationTarget.setCustomValidity(this.customInvalidText || "An error occurred");
this.setCustomValidity(this.customInvalidText || "An error occurred");
Copy link
Member

Choose a reason for hiding this comment

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

we have localized text here, either remove it or wrap with localization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It felt like a different context bro

Copy link
Member

Choose a reason for hiding this comment

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

it'd be nice to solve it as we caught it, but fine to handle it in a separate pull request.

@@ -13,31 +13,58 @@ class MyInvalidInput extends LitElement {
@query("input")
validationTarget: HTMLInputElement;

@property({ reflect: true, type: String })
error: string = "";
Copy link
Member

Choose a reason for hiding this comment

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

no need to give strict type, already inferred

validationTarget: HTMLInputElement;

@property({ reflect: true, type: String })
error: string = "";
Copy link
Member

Choose a reason for hiding this comment

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

no need to give strict type

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.

5 participants