Conversation
Looks good. Thanks for your work on this! The one thing that I'll say is that you'll definitely need polyfills for things like |
@@ -16,7 +16,7 @@ module.exports = ngModule => { | |||
restrict: 'E', | |||
template: function(el, attrs) { | |||
/* jshint -W033 */ // this because jshint is broken I guess... | |||
var rootEl = attrs.rootEl || 'ng-form'; | |||
var rootEl = attrs.rootEl || 'form'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this. We can't do this. This would be a breaking change. Also, we want to default to ng-form
because form
s cannot be nested, and that's a pretty big use case for angular-formly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, would you be fine with it to only do that when running in IE8?
Or do a <div ng-form> when running in IE8?
IE8 can't cope with ng-form and directive replace because for some reason custom tags in IE8 look like having multiple root nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely don't want to have it change this kind of behavior in different browsers. That could lead to some really odd bugs. Why couldn't we simply add 'ng-form'
to the list of custom elements in the formlyCustomTags
file you've made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly that doesn't work, even when you add ng-form to the custom tags. But I'll try to find another way. Stay tuned! ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kentcdodds Would you be fine with this change:
var formTag = (attrs.rootEl || 'ng-form').split(' ');
var formEl = formTag[0];
var formAtt = formTag.slice(1).join(' ');
return `
<${formEl} ${formAtt} class="formly">
</${formEl}>
`;
That way, the default is still 'ng-form', and users can specify things like root-el="form"
or root-el="div ng-form"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather not overload the rootEl
attribute to do this. I think the current implementation allows you to deal with the issue just fine. If you want to use an ng-form
but you don't want it to be the root element, then you can do:
<div ng-form>
<formly-form root-el="div">
</formly-form>
</div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right, I'll close this PR and will open a new one with these lines reverted.
I did have to add quite a few polyfills (especially for api-check), but I'm guessing you would want to keep those out of both api-check and angular-formly. |
@koenweyn, you've guessed right :-) |
@koenweyn, seriously, thank you for working on this. I really appreciate the help! I hope it's worth it for you! |
@kentcdodds, I'm just really keen on successfully completing my first ever github pull request ;-) |
Me too! Were you there?! Did I meet you? |
Thanks for being a good bridge crosser/builder! :D |
I was there, but we didn't meet, I'll come and say hi next time. |
Definitely :D |
Hey @koenweyn, I'm writing a blogpost that references this PR. Do you mind if I reference you in it? https://medium.com/@kentcdodds/78281ea47455 |
Sure, no problem
Koen
|
Sadly I have to support IE8 for our project, so I spent the day trying to get formly to work.
I pulled it off by adding the necessary custom tags, and using a different method of locating tags.
I could not run the unit tests in IE8, because chai doesn't support it, but I can confirm that my formly form now works.