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

Rename IPropertyPaneTextFieldProps.onGetErrorMessage to IPropertyPaneTextFieldProps.GetValidationMessage #90

Closed
vman opened this Issue Aug 27, 2016 · 4 comments

Comments

Projects
None yet
7 participants
@vman
Copy link
Contributor

vman commented Aug 27, 2016

According to the docs, the onGetErrorMessage function is used to determine whether the value of a TextField in the property pane is valid or not. The function returns an error message if value is invalid and returns an empty string if value is valid.

In that case, should it be renamed to something else like GetValidationMessage which makes it clear that the function will return a validation message. The name onGetErrorMessage makes it sound like it will always return an error message.

Also, I don't think the property should start with the word "on". That makes it seem like the associated function would fire when an error message will be received. E.g. An onChange function fires when a change event occurs. Similarly, we expect onGetErrorMessage function to fire when an error message is received which is not the case here.

Here is my sample code:

PropertyPaneTextField('description', {
        label: strings.DescriptionFieldLabel,
        onGetErrorMessage: this._validateDescription
 })

private _validateDescription(value: string): string {
    // If validation is not successful, return a string with error message.
    if (value.length < 10) {
      return "At least 10 characters required";
    }
    else {
      // If validation is successful, return an empty string.
      return "";
    }
  }
@nickpape-msft

This comment has been minimized.

Copy link
Contributor

nickpape-msft commented Sep 3, 2016

@levalencia

This comment has been minimized.

Copy link
Contributor

levalencia commented Sep 18, 2016

Agree, the name of the method assumes an error happened :)

@wictorwilen

This comment has been minimized.

Copy link
Contributor

wictorwilen commented Sep 19, 2016

While we're at it. It should not be named onGetErrorMessage. That assumes that we're expecting an event when we get the error message. Proper naming should bet onError or rather onValidation, ie it fires on validation.

@levalencia

This comment has been minimized.

Copy link
Contributor

levalencia commented Sep 19, 2016

OnValidation seems to be the correct one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.