Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

md-input directive #848

Closed
wants to merge 2 commits into from
Closed

md-input directive #848

wants to merge 2 commits into from

Conversation

ajoslin
Copy link
Contributor

@ajoslin ajoslin commented Dec 4, 2014

Old problem: We didn't know what attributes to pass down to the input directive from the input container.

Solution: We have a directive with priority 100 and terminal: true. terminal means that no directive with lower priority will run on this element - basically, directive compilation will stop at md-input.

With priority: 100, md-input still runs after ng-if, ng-repeat, etc.

When md-input runs, it takes all attributes on <md-input> container and copies them down to a child <input> element - EXCEPT attributes that are directives with higher priority (like ng-if).

PROBLEMS

  • ng-show is actually 0 priority, so it will be passed down to the input element. That means the input element will be hidden, but not the parent.

@googlebot
Copy link

CLAs look good, thanks!

// Copy relevant attributes down to the input element: attrs that either aren't a directive,
// or are a directive that has already run (a directive with higher priority)
// For example, ng-if would NOT be copied down to the input because it has a higher priority
angular.forEach(node.attributes, function(data) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could attr be used instead of node.attributes ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there are other implications if attr was used (e.g. using getAttribute() to get the original (non-interpolated) value of the attribute etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I should be using the compile function to get the un-interpolated values.

@gkalpak
Copy link
Member

gkalpak commented Dec 4, 2014

@ajoslin: Have you evaluated the alternative of having a specific prefix for "transferrable" attributes ? Are there any drawbacks to that (which I might not have thought of) ?

E.g.:

<md-input ng-show="someCondition" md-x-ng-pattern="test" md-x-title="This is copied"></md-input>
<!-- `ng-show` stays on the parent -->
<!-- `md-x-ng-pattern` gets copied to child input (as `ng-pattern`) -->
<!-- `md-x-title` as `title` -->

Pros:

  1. Don't have to "fight" with priorities. Less maintainance (e.g. we don't have to worry about new directives being introduced).
  2. More flexible for the user, who doesn't have to worry about the priorities of their custom directives.
  3. The behaviours intended for the child input, aren't applied to the parent (e.g. title).
  4. The behaviours intended for the parent, aren't applied to the child (regarding non-directive attributes).
  5. This logic can be reused with similar components (e.g. md-button, md-text-float that create control elements), without having to have explicit or special handling for each.

Cons: ??

@ajoslin
Copy link
Contributor Author

ajoslin commented Dec 4, 2014

@gkalpak interesting idea!

The con of course is that the developer won't be able to use the attribute names he is accustomed to.

Hmm..

@ThomasBurleson
Copy link
Contributor

I do not thinks this is a good idea as I worry that developers will have to learn which attributes (with prefixes) are supported... this is a verbose solution instead of concise.

@ajoslin
Copy link
Contributor Author

ajoslin commented Dec 4, 2014

The problem of ngShow and similar directives is a very annoying one...

Directives with higher priority execute on the parent element, not the input: this makes sense.

Directives with 0 priority that should execute on the parent element: what to do?

@gkalpak
Copy link
Member

gkalpak commented Dec 5, 2014

I do not thinks this is a good idea as I worry that developers will have to learn which attributes (with prefixes) are supported... this is a verbose solution instead of concise.

Actually, it is the other way around (maybe I wasn't clear enough).
What I propose is that ALL attributes can be made available on either the original element (no prefix) or on the generated input (with prefix). The developers don't have to remember anything (except for the prefix).
On the other hand, using the priorities-based approach:

  1. The developers have to remember the priority of each directive.
  2. The developers have no way to specify attributes (non-directives) for the generated input (e.g. title, autocomplete etc).
  3. Directive priorities may not be what we want (e.g. ngShow, third-party directives).
  4. The code is at constant risk of breaking, because of some unrelated changes in another project (e.g. AngularJS changes the priority of a directive, introduces a new directive with "unconvenient" priority etc).

This coupling between unrelated things (directive priorities, attributes transfered) is only going to create headaches for both the developers and maintainers alike.

That said, (and despite me being all against verbosity in general), I do believe that the verbosity in my proposal is minimal and justified by the benefits*.

*: In fact it's not exactly benefits; it's avoiding the alternative solution's drawbacks. A totally different approach might be way better that both alternatives.

@gkalpak
Copy link
Member

gkalpak commented Dec 5, 2014

Talking about alternative approaches, here are a couple more suggestions
(not sure I like either of them better than the prefix suggested earlier, but laying them on the table anyway):

  1. Use an ultra high priority and copy all attributes (directives or not) on the generated input (before the y execute on the original element). If someone wants to use directives on the "group", should wrap the element and use them on the wrapper. Document this behaviour clear and loud.

    <md-input ng-if="..." ...></md-input>   <!-- Applies to the generated input -->
    
    <div ng-if="..."><md-input ...></md-input></div>   <!-- Applies to the whole input group -->

    This is not a good approach if applying attributes to the original element is a frequent use-case.

  2. Allow (but not require) for an input element to be placed inside of <md-input>. If such an element is provided, it will either act as "template" or augmented and used as the actual input. This way, you can specify the attributes you want your input to have.

    <md-input ng-if="..."></md-input>    <!-- Applies to the original element -->
    
    <md-input><input ng-if="..."/></md-input>    <!-- Applies to the generated input -->

    This is not a good approach if applying attributes to the generated input is a frequent use-case.

  3. Either apply all attributes/directives on the original element or all on the generated input, based on a boolean "flag" attribute

    <md-input ng-if="..."></md-input>    <!-- Applies to the original element -->
    
    <md-input ng-if="..." apply-to-input></md-input>    <!-- Applies to the generated input -->

    This is not a good approach if you want to apply some attributes on the original element and some on the generated input.

@caitp
Copy link
Contributor

caitp commented Dec 5, 2014

As @gkalpak has mentioned, I've brought this up for 1.x --- and a few weeks ago, proposed this for 2.x as an alternative to the template attribute craziness (just because I think it will be less of a headache for people to use the key/value system baked into the platform, rather than a special directive with a microsyntax).

If we did move this into 1.x, it could work well here

@ajoslin ajoslin added wip and removed in progress labels Dec 8, 2014
@demetriusnunes
Copy link

I support the attribute prefix approach for passing it down to the concrete elements, specially when you consider that there is also a "label" element inside the md-text-float directive. How do we know which attributes should be passed down to the label or to the input? If we agree on a simple naming convention, for instance: md-text-float-input-attr- and md-text-float-label-attr- then developers won't have to learn new attributes, just know the naming convention. And there's no other complications such as priorities, transclusions, etc.

@ilanbiala
Copy link
Contributor

I think it should support regular attributes, as those are what people are used to and will minimize confusion.

var input = angular.element(element[0].querySelector(
(angular.isDefined(attr.mdMultiline) ? 'textarea' : 'input') + ':last-of-type'
));
var ngModelCtrl = input.data('$ngModelController');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be:

var ngModelCtrl = input.controller('ngModel');

Or does that look at inherited data too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does --- but for ng-model that does not seem like an issue, in general

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks at inherited data. I just did that incase a parent has a model and the input doesn't.

@ThomasBurleson ThomasBurleson changed the title WIP: md-input directive md-input directive Dec 9, 2014
@ajoslin
Copy link
Contributor Author

ajoslin commented Jan 8, 2015

Went with a different approach. 60fcd6f

@ajoslin ajoslin closed this Jan 8, 2015
@marcysutton marcysutton deleted the wip-input branch April 10, 2015 00:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants