Skip to content
This repository has been archived by the owner on Apr 30, 2018. It is now read-only.

Add option for templateManipulator to ignore specific ng-model #358

Closed
mikejpeters opened this issue Jun 23, 2015 · 8 comments
Closed

Add option for templateManipulator to ignore specific ng-model #358

mikejpeters opened this issue Jun 23, 2015 · 8 comments

Comments

@mikejpeters
Copy link

Sometimes custom templates may involve multiple directives referring to the same ng-model. For example:

<div class="dropdown">
    <input ng-model="model[options.key]">
    <a data-toggle="dropdown">Click here to show calendar</a>
    <ul class="dropdown-menu">
        <datetimepicker ng-model="model[options.key]"></datetimepicker>
    </ul>
</div>

In this situation, formly adds the same attributes to both the input and datetimepicker.

We can tell the manipulator to skip ALL attributes using skipNgModelAttrsManipulator, but what we really want is to just ignore the one on datetimepicker.

It would be great to be able to add an attribute to the actual element so it is ignored:

<datetimepicker ng-model="model[options.key]" formly-skip-ng-model-attrs-manipulator>
@kentcdodds
Copy link
Member

Very good. I like the suggestion. I'd be happy to pull this concept in. I'll try to get to it as soon as I am able. If you want to try your hand at a PR, see CONTRIBUTING.md and the changes will likely be around here: https://github.com/formly-js/angular-formly/blob/master/src/run/formlyNgModelAttrsManipulator.js#L17

@kentcdodds
Copy link
Member

Like with #343, I've created tests for this feature and I'm going to give someone a shot at contributing to open source for the first time :-) So I will only accept a PR from someone who's never contributed to open source before.

Instructions:

  • Watch this video to learn what you need to do to get things setup
  • Go to these tests and change it.skip to it.only
  • Run $ npm test
  • Notice that all the tests are failing
  • Update the formlyNgModelAttrsManipulator to make the tests pass. (hint, what you'll want to do is construct the query sent into querySelectorAll to add :not for both the special attribute as well as the specified selector, so you can potentially have two :nots in your query.)
  • Once they're passing, change the it.onlys to its so all the tests run.
  • Commit your changes to the src/ directory mentioning issue Add a hook to transform field config before forms are compiled #343 (notice it runs all the tests... twice)
  • Push your changes to your fork, create a PR, get merged, celebrate 🎉

I'm happy to hold your hand through this if you need help. Catch me on gitter.

kentcdodds pushed a commit that referenced this issue Jun 25, 2015
@kentcdodds
Copy link
Member

P.S. I've already developed the solution, I just haven't committed it because I want to give someone else a first shot at contributing to open source 👍

ccasad added a commit to ccasad/angular-formly that referenced this issue Jun 25, 2015
@ccasad ccasad mentioned this issue Jun 25, 2015
ccasad added a commit to ccasad/angular-formly that referenced this issue Jun 25, 2015
ccasad added a commit to ccasad/angular-formly that referenced this issue Jun 25, 2015
@kentcdodds
Copy link
Member

@douglas-mason committed this! Thanks!

@kentcdodds
Copy link
Member

This will be released as soon as I'm able

@kentcdodds
Copy link
Member

This has been officially released in 6.16.0. Docs have been updated.

@koenweyn
Copy link
Contributor

koenweyn commented Aug 7, 2015

Hi @kentcdodds, the fix for this breaks IE8 compatibility.
querySelectorAll(':not()') is not supported, because it is a CSS3 selector: Can I Use?.
I have implemented a fix that builds the nodelist in a different way when the current method throws an Exception.
Would you be interested in that PR? Or are you tired of trying to support IE8? ;-)

@kentcdodds
Copy link
Member

Would you be interested in that PR? Or are you tired of trying to support IE8?

Both :-) Thanks!

koenweyn added a commit to acsl/angular-formly that referenced this issue Aug 7, 2015
kentcdodds pushed a commit that referenced this issue Aug 7, 2015
Fix for #358 that works around the fact that IE8 does not support querySelectorAll(':not()')
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants