ng1bs4 icon indicating copy to clipboard operation
ng1bs4 copied to clipboard

Add Validations and Error Handling

Open IdanCo opened this issue 7 years ago • 1 comments

Implement binding validations in all components.

Adhere @langdonx 's implementation in Pagination component:

...
 $onInit() {
    // validate bindings
    if (typeof this.currentPage !== 'number' || isFinite(this.currentPage) === false) {
      this.$log.error('invalid ngbsPagination::currentPage:', this.currentPage, 'expecting a number');
    }
    ...
}

IdanCo avatar Aug 04 '17 12:08 IdanCo

I mentioned this here, but it would be so much easier to implement these everywhere if we had some nice utility functions. What do you think about a validation.service.js? I'm not sure where it lives (maybe right ./src/library/), but it looks like this:

export default class {
  constructor($log) {
    this.$log = $log;
  }

  validateNumber(obj, component, attribute) {
    const property = convertCamelToHyphenated(attribute);

    if (obj[property] && (typeof obj[property] !== 'number' || isFinite(obj[property]) === false)) {
      this.$log.error(`invalid ${component}::${attribute}: ${JSON.stringify(obj[property])} expecting a number`);
    }
  }

  validateEtc() {}

  validateEtc() {}

  validateEtc() {}
}

And then it's implemented like this for progress.component.js:

$onChanges(changesObj) {
  ValidationService.validateNumber(changesObj, 'ngbs-progress', 'progress-max');
  ValidationService.validateNumber(changesObj, 'ngbs-progress', 'progress-value');
  ValidationService.validateBoolean(changesObj, 'ngbs-progress', 'progress-animated-progress');
  ValidationService.validateBoolean(changesObj, 'ngbs-progress', 'progress-auto-label');
  ValidationService.validateBoolean(changesObj, 'ngbs-progress', 'progress-striped');
  ValidationService.validateBoolean(changesObj, 'ngbs-progress', 'progress-animated-striped');
  ValidationService.validateStringFromArray(changesObj, 'ngbs-progress', 'progress-background', SUPPORTED_BACKGROUNDS);
}

Some thoughts:

  • 26 less lines in the component!
  • I'm passing changesObj to the ValidationService (instead of this) because we don't need to validate all the attributes every any one of them changes.
  • I switched from passing 'camelCaseAttribute' to 'component-case-attribute' because a user will be seeing these errors and I imagine it'll be easier to read since that's how they reference them. I don't really know if it's worth the cost in efficiency to be doing that conversion. I imagine for some components $onChanges might get fired a lot.

langdonx avatar Aug 09 '17 04:08 langdonx