fix(ngList) Fix model to view rendering when using a custom separator #2561
Conversation
This change adds a new ng-list-joined-by attribute to the ngList directive.
This allows you to specify a custom string to join elements of the model
array together which is useful when you are also using a custom separator.
Example:
<input type="text" ng-model="model.values" ng-list="|", ng-list-joined-by=" | ">
This will split an input string such as "Cat | Mouse | Dog" into the array
["Cat", "Mouse", "Dog"] and vice versa.
| separator = match && new RegExp(match[1]) || attr.ngList || ','; | ||
| // Get the attribute values directly from the element rather than the | ||
| // attr map otherwise the attribute values will be trimmed of whitespace | ||
| var separatorAttribute = element[0].getAttribute("ng-list") |
There was a problem hiding this comment.
OK, so you want get the attribute directly because of the trimming but you need to use the attr.$attr object to get the original attribute name from the normalized version, in the case that the user has use data-ng-list or some other acceptable format.
http://docs.angularjs.org/api/ng.$compile.directive.Attributes#$attr
e.g.
var separateorAttribute = element[0].getAttribute(attr.$attr['ngList']);
|
|
This fixes a problem caused when ng-list attributes have non standard but valid names such as ng:list or data-ng-list
|
The last three commits should address your issues, @petebacondarwin It does make the whole pull request a little messier though, should I squash all the commits into one or leave them as they are? |
|
I can rebase and squash. I have fixed a couple of style issues. |
|
|
Just wondering now, whether changing the directive not to trim the ng-list attribute is actually a breaking change. It is clearly not likely that someone would rely on this but it would change the behaviour of the directive... |
|
Arguably it was a bug in the original |
|
I would argue it was a bug. For example, if you specified a separator of " or " and a string "House or Door or Chair", it would previously get split into ["House", "Do", "", "Chair"]. With this change it should split correctly into three items. However this could still potentially break applications for people who for some reason set the separator to say "; " and then expect the be able to input strings such as "Cat;Dog;Mouse". The only reason I can think someone might have done this is if they expected "; " to be used as the join string when going from the model to view but seeing as currently the code will always use the string ", " then this functionality would currently be broken for such a user. |
|
But then again, generally whitespace is ignored when splitting stuff and so If whitespace is important to the splitter then there is already a workaround - to use a regex: `/ or /', where the spaces are taking into account. With your new attribute, there is a clear way to specify how to join regex separated lists back together. For backwards compatibility I think this should be maintained but the documentation should be updated to make it clear how to deal with situation where whitespace is important. @IgorMinar - do you have a view here? |
|
BTW, I find it a bit weird that we can separate with one string and join with another completely different one, as you do in your tests. This would lead to strange behaviour as soon as you have the potential to update both the model and the view in one page. I wonder if really join should only be available in two cases:
|
| @@ -1225,6 +1225,8 @@ var requiredDirective = function() { | |||
| * @element input | |||
| * @param {string=} ngList optional delimiter that should be used to split the value. If | |||
| * specified in form `/something/` then the value will be converted into a regular expression. | |||
| * @param {string=} ngListJoin optional string to use when joining elements of the array. | |||
There was a problem hiding this comment.
I think this should be just trim of the ngList. You can not have different join and separator values.
|
I think the dichotomy between the split and join is something we should avoid. I propose getting rid of ng-list-joined-by and just become more intelligent about ng-list whitespace. |
|
@mhevery, what about the case where the separator is a regex? There is no way we can intelligently guess what the join string should be in this case. The problem is not just a matter of being intelligent about white space. My proposal does allow the user to be inconsistent with the separator and join strings but it gives complete control over the behaviour to the user rather than forcing any particular behaviour on them. I think I agree with @petebacondarwin that we need a join string in the following situations (I assume pete meant != instead of == in the following conditions):
And in the case of say separator = ";" and join = "; " we can use some intelligence about how to handle the whitespace. I will try to come up with a full set of use cases that I think we should be able to support |
|
+1 @alexspurling, except I actually meant |
|
@petebacondarwin ok that makes sense. Here are all the use cases I can think of that we need to handle and what I expect the model and view behaviour to be:
This logic is:
Please let me know if you agree with these rules or if you think there are any use cases that I've missed. If we are agreed then I'll start work on updating the code to implement it. |
|
This seems reasonable to me. @mhevery - what do you think? On 3 May 2013 17:36, Alex Spurling notifications@github.com wrote:
|
|
I think we have two options here: 1/ simple
2/ complex
I'm leaning towards 1/ because ngList is not a highly used directive and adding features like this is not making it significantly better, but it does cost us bytes and maintenance cost. |
|
Personally I would go with 1/ as well. I would be even tempted to consider removing it from the core.... |
|
ok, let's go with 1/ can we get this PR updated to reflect that, please? |
|
Closing due to inactivity, and speculative nature of the request. Both @IgorMinar and @petebacondarwin agree that we should go with simple solution and let third party take a complicated solutions. Reopen if you feel otherwise and you have addressed the concerns of this PR discussion. |
|
@mhevery - So are we going to implement the simple solution? At the moment the joining nature of ng-list is very broken. |
Remove support for regexp separators, and correctly use custom separator when formatting values into the control. See angular#4008 and angular#2561.
The separator string used to split the view value into a list for the model value is now used to join the list items back together again for the view value. BREAKING CHANGE: The `ngList` directive no longer supports splitting the view value via a regular expression. We need to be able to re-join list items back together and doing this when you can split with regular expressions can lead to inconsistent behaviour and would be much more complex to support. If your application relies upon ngList splitting with a regular expression then you should either try to convert the separator to a simple string or you can implement your own version of this directive for you application. Closes angular#4008 Closes angular#2561 Closes angular#4344
The separator string used to split the view value into a list for the model value is now used to join the list items back together again for the view value. BREAKING CHANGE: The `ngList` directive no longer supports splitting the view value via a regular expression. We need to be able to re-join list items back together and doing this when you can split with regular expressions can lead to inconsistent behaviour and would be much more complex to support. If your application relies upon ngList splitting with a regular expression then you should either try to convert the separator to a simple string or you can implement your own version of this directive for you application. Closes angular#4008 Closes angular#2561 Closes angular#4344
The separator string used to split the view value into a list for the model value is now used to join the list items back together again for the view value. BREAKING CHANGE: The `ngList` directive no longer supports splitting the view value via a regular expression. We need to be able to re-join list items back together and doing this when you can split with regular expressions can lead to inconsistent behaviour and would be much more complex to support. If your application relies upon ngList splitting with a regular expression then you should either try to convert the separator to a simple string or you can implement your own version of this directive for you application. Closes #4008 Closes #2561 Closes #4344
This change adds a new ng-list-joined-by attribute to the ngList directive.
This allows you to specify a custom string to join elements of the model
array together which is useful when you are also using a custom separator.
Example:
This will split an input string such as "Cat | Mouse | Dog" into the array
["Cat", "Mouse", "Dog"] and vice versa.