-
Notifications
You must be signed in to change notification settings - Fork 216
feat(selector): attribute normalization #603
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,10 +17,18 @@ class NodeAttrs { | |
|
||
NodeAttrs(this.element); | ||
|
||
operator [](String attributeName) => | ||
element.attributes[attributeName]; | ||
operator [](String attributeName) { | ||
var value = element.attributes[attributeName]; | ||
if (value == null && attributeName.startsWith('ng-')) { | ||
value = element.attributes['data-$attributeName']; | ||
} | ||
return value; | ||
} | ||
|
||
operator []=(String attributeName, String value) { | ||
if (attributeName.startsWith('ng-')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, ng- should not be special |
||
element.attributes.remove('data-$attributeName'); | ||
} | ||
if (value == null) { | ||
element.attributes.remove(attributeName); | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,31 @@ part of angular.core.dom; | |
*/ | ||
typedef List<DirectiveRef> DirectiveSelector(dom.Node node); | ||
|
||
String _normalizeKey(String key) => | ||
key.startsWith('data-ng-') ? key.substring('data-'.length) : key; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove ng- |
||
|
||
class _AttrValueMap<V> implements Map<String, V> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this? Why do we need to subclass Map? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I introduced _AttrValueMap because I'd like to ensure both an attrValueMap property and an attrValuePartialMap property in _ElementSelector class never hold data- attributes. But now I notice there are only a few places where attrValueMap and attrValuePartialMap are set, so you are right. Introducing new class may be too much and it should be eliminated. |
||
var _map = <String, V>{}; | ||
|
||
void addAll(Map<String, V> other) { _map.addAll(other); } | ||
bool containsKey(String key) => _map.containsKey(_normalizeKey(key)); | ||
bool containsValue(Object value) => _map.containsValue(value); | ||
void clear() { _map.clear(); } | ||
void forEach(Function f) { _map.forEach(f); } | ||
V putIfAbsent(String key, Function ifAbsent) => | ||
_map.putIfAbsent(_normalizeKey(key), ifAbsent); | ||
V remove(String key) => _map.remove(_normalizeKey(key)); | ||
|
||
V operator[](String key) => _map[_normalizeKey(key)]; | ||
void operator[]=(String key, V value) { _map[_normalizeKey(key)] = value; } | ||
|
||
bool get isEmpty => _map.isEmpty; | ||
bool get isNotEmpty => _map.isNotEmpty; | ||
int get length => _map.length; | ||
Iterable<String> get keys => _map.keys; | ||
Iterable<V> get values => _map.values; | ||
} | ||
|
||
class _Directive { | ||
final Type type; | ||
final NgAnnotation annotation; | ||
|
@@ -76,7 +101,6 @@ class _SelectorPart { | |
: element; | ||
} | ||
|
||
|
||
class _ElementSelector { | ||
final String name; | ||
|
||
|
@@ -86,8 +110,8 @@ class _ElementSelector { | |
var classMap = <String, List<_Directive>>{}; | ||
var classPartialMap = <String, _ElementSelector>{}; | ||
|
||
var attrValueMap = <String, Map<String, List<_Directive>>>{}; | ||
var attrValuePartialMap = <String, Map<String, _ElementSelector>>{}; | ||
var attrValueMap = new _AttrValueMap<Map<String, List<_Directive>>>(); | ||
var attrValuePartialMap = new _AttrValueMap<Map<String, _ElementSelector>>(); | ||
|
||
_ElementSelector(this.name); | ||
|
||
|
@@ -175,7 +199,7 @@ class _ElementSelector { | |
dom.Node node, String attrName, | ||
String attrValue) { | ||
|
||
String matchingKey = _matchingKey(attrValueMap.keys, attrName); | ||
String matchingKey = _matchingKey(attrValueMap.keys, _normalizeKey(attrName)); | ||
|
||
if (matchingKey != null) { | ||
Map<String, List<_Directive>> valuesMap = attrValueMap[matchingKey]; | ||
|
@@ -306,6 +330,8 @@ DirectiveSelector directiveSelectorFactory(DirectiveMap directives) { | |
// we need to pass the name to the directive by prefixing it to | ||
// the value. Yes it is a bit of a hack. | ||
directives[selectorRegExp.annotation].forEach((type) { | ||
if (attrName.startsWith('data-ng-')) | ||
attrName = attrName.substring('data-'.length); | ||
directiveRefs.add(new DirectiveRef( | ||
node, type, selectorRegExp.annotation, '$attrName=$value')); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ part of angular.directive; | |
class NgCloakDirective { | ||
NgCloakDirective(dom.Element element) { | ||
element.attributes.remove('ng-cloak'); | ||
element.attributes.remove('data-ng-cloak'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In #560, you wrote we should not iterate over the element attributes to replace data-foo attributes to foo attributes. So if we have to accept users to use data- attributes, I think we should check about data-attributes in each directives which depends on a specific attribute. |
||
element.classes.remove('ng-cloak'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,9 +95,15 @@ class NgAttributeDirective implements NgAttachAware { | |
|
||
void attach() { | ||
String ngAttrPrefix = 'ng-attr-'; | ||
String dataNgAttrPrefix = 'data-ng-attr-'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Existing directives should be blissfully unaware of any changes to the data- prefix. The fact that you have to change this, means that the abstraction is wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as previous |
||
_attrs.forEach((key, value) { | ||
var newKey; | ||
if (key.startsWith(ngAttrPrefix)) { | ||
var newKey = key.substring(ngAttrPrefix.length); | ||
newKey = key.substring(ngAttrPrefix.length); | ||
} else if (key.startsWith(dataNgAttrPrefix)) { | ||
newKey = key.substring(dataNgAttrPrefix.length); | ||
} | ||
if (newKey != null) { | ||
_attrs[newKey] = value; | ||
_attrs.observe(key, (newValue) => _attrs[newKey] = newValue ); | ||
} | ||
|
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.
We should not be special casing ng- any attribute can have a data- not just ng-
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.
This means users cannot define their own directive using a data- attribute as a selector. Is it a spec that all data-foo attributes are taken as foo attributes in all directive selectors?