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

support latest version of ember-changeset / ember-changeset-validations #27

Merged

Conversation

jelhan
Copy link
Collaborator

@jelhan jelhan commented Jul 19, 2020

This fixes two issues:

  1. It adds support for latest version of ember-changeset. It does so by not relying anymore on private API but using a native getter together with the compatibility layer dependentKeyCompat for computed properties. This can be removed as soon as FormElement class of Ember Bootstrap is refactored to use tracked properties and/or to provide a different API for validation plugins. This was reported as Broken with latest version of ember-changeset / ember-changeset-validations #26.
  2. It fixes a bug with validation of initial state. Ember Changeset does not validate the initial state. A property is not validated at all until a) it's set or b) validate() method is called for it (or for all properties). This conflicts with Ember Bootstrap's assumption that validation is always up to date. A form element may be shown as valid just cause no validation was run ever. This was reported as Focus out is causing incorrect display of success on form element #22.

Fixes #26 and #22.

@jelhan
Copy link
Collaborator Author

jelhan commented Jul 19, 2020

@simonihmig The current API for validation plugins does not seem to be flexible enough for common use cases. Or did I missed an obvious alternative to overriding showValidationOnHandler?

@jelhan jelhan force-pushed the support-latest-ember-changeset-versions branch from 8cc94f7 to 21636fd Compare July 20, 2020 11:45
@jelhan
Copy link
Collaborator Author

jelhan commented Jul 20, 2020

Rebased after #25 has been merged.

@jelhan jelhan added the bug label Jul 20, 2020
@jelhan jelhan merged commit a35bfce into ember-bootstrap:master Jul 20, 2020
@basz
Copy link
Contributor

basz commented Jul 20, 2020

Hi, could it be that this change affects the displaying of nested properties element groups? After upgrading I seem to have forms not displaying errors for nested properties, while json this.changeset.errors clearly shows them to error. Same form with root properties do show the errors...

@jelhan
Copy link
Collaborator Author

jelhan commented Jul 20, 2020

Yes. That could be caused by this change. Before it was using Ember.get(), which supports nested property readings: https://github.com/kaliber5/ember-bootstrap-changeset-validations/blob/c13b1d24557abcec3eb57726af831a4617493bb1/addon/components/bs-form/element.js#L17-L20

Refactored away from that one: https://github.com/kaliber5/ember-bootstrap-changeset-validations/blob/9034f0296e03d95fb61c99733d29b515178985c3/addon/components/bs-form/element.js#L10

Haven't considered that property may be a path itself. I think we need to use get to support that. Something like:

+ import { get } from '@ember/object';

- let error = this.model?.error?.[this.property]?.validation;
+ let error = get(this, `model.error.${this.property}.validation`);

Do you have time for a bug fix?

@basz
Copy link
Contributor

basz commented Jul 20, 2020

this seems a legit fix, as that does return the correct validation message. The field is not rendered accordingly though...

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

Successfully merging this pull request may close these issues.

Broken with latest version of ember-changeset / ember-changeset-validations
2 participants