Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ngList) Fix model to view rendering when using a custom separator #2561

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@alexspurling
Copy link

alexspurling commented May 1, 2013

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.

Alex Spurling added some commits May 1, 2013

Alex Spurling
fix(ngList) Fix model to view rendering when using a custom separator
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")

This comment has been minimized.

@petebacondarwin

petebacondarwin May 2, 2013

Member

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']);
@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 2, 2013

ng-list-joined-by seems a bit verbose. Perhaps we could go with ng-list-join?

Alex Spurling added some commits May 2, 2013

Alex Spurling
fix(ngList) get ng-list attribute names via attr.$attr
This fixes a problem caused when ng-list attributes have non standard but valid
names such as ng:list or data-ng-list
@alexspurling

This comment has been minimized.

Copy link
Author

alexspurling commented May 2, 2013

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?

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 2, 2013

I can rebase and squash. I have fixed a couple of style issues.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 2, 2013

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented
@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 2, 2013

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...

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 2, 2013

Arguably it was a bug in the original

@alexspurling

This comment has been minimized.

Copy link
Author

alexspurling commented May 3, 2013

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.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 3, 2013

But then again, generally whitespace is ignored when splitting stuff and so ', ' would nicely split "Dog,Cat", "Dog, Cat" and "Dog , Cat". Which some people might expect.

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?

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 3, 2013

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:

  1. You used a regex for the separator
  2. trim(separator) == joiner
@@ -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.

This comment has been minimized.

@mhevery

mhevery May 3, 2013

Member

I think this should be just trim of the ngList. You can not have different join and separator values.

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(attr.$attr['ngList'])

This comment has been minimized.

@mhevery

mhevery May 3, 2013

Member

this should be: attr.ngList not attr.$attr.ngList

This comment has been minimized.

@alexspurling

alexspurling May 3, 2013

Author

See the discussion on whitespace above for why I use getAttribute here.

// 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(attr.$attr['ngList'])
var joinAttribute = element[0].getAttribute(attr.$attr['ngListJoin'])

This comment has been minimized.

@mhevery

mhevery May 3, 2013

Member

same here



it('should allow custom separator and custom join string', function() {
compileInput('<input type="text" ng-model="list" ng-list=":" ng-list-join=" and "/>');

This comment has been minimized.

@mhevery

mhevery May 3, 2013

Member

this makes no sense. How can you split on ':' and join on " and ". This means that during edit the text would change on you if someone changes the model. This is undesirable. I really think that there should be on ng-list and and some intelligent logic about how to ignore whitespace. So if it is work white space is added, but otherwise it is ignored or something like that.

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented May 3, 2013

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.

@alexspurling

This comment has been minimized.

Copy link
Author

alexspurling commented May 3, 2013

@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):

  1. You used a regex for the separator
  2. trim(separator) != joiner

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

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 3, 2013

+1 @alexspurling, except I actually meant trim(sep) == trim(joiner). What I am saying is that if the separator is not a regex AND the trimmed separator is not the same as the trimmed joiner then we should throw an error.

@alexspurling

This comment has been minimized.

Copy link
Author

alexspurling commented May 3, 2013

@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:

#ng-listng-list-joininputmodelview
1undefinedundefinedRed,Blue, Green["Red", "Blue", "Green"]Red, Blue, Green
2", "undefinedRed,Blue, Green["Red", "Blue", "Green"]Red, Blue, Green
3";"undefinedRed;Blue; Green["Red", "Blue", "Green"]Red;Blue;Green
4"; "undefinedRed;Blue; Green["Red", "Blue", "Green"]Red; Blue; Green
5";""; "Red;Blue; Green["Red", "Blue", "Green"]Red; Blue; Green
6"/[,;]\s*/""; "Red; Blue, Green["Red", "Blue", "Green"]Red; Blue; Green
7"/[,;]\s*/"undefinedRed; Blue, Green["Red", "Blue", "Green"]Compile Error
8"/ or /"" or "House or Door or Car["House", "Door", "Car"]House or Door or Car
9"/[,;]\s*/""-"Red; Blue, Green["Red", "Blue", "Green"]Red-Blue-Green
10";"","Red; Blue; Green["Red", "Blue", "Green"]Compile Error

This logic is:

  • If the separator and join string are undefined, the separator is "," and the join string is ", " (1)
  • If the separator is specified and the join string is undefined, the join string is the same as the separator (2,3,4)
  • If the separator has whitespace, it will be trimmed (4)
  • If the join string is specified and trim(separator) == trim(joinstring), the untrimmed join string is used to render the view (5)
  • If a regular expression is used then a join string must be specified (6,7)
  • If you want the separator to consider whitespace, use a regular expression + join string (8)
  • There is no protection if you decide to use an inconsistent join string with your regular expression (9)
  • If both the separator and join string are specified and trim(separator) != trim(join) then an error is thrown (10)

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.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented May 3, 2013

This seems reasonable to me. @mhevery - what do you think?

On 3 May 2013 17:36, Alex Spurling notifications@github.com wrote:

@petebacondarwin https://github.com/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:

ng-list ng-list-join input model view 1 undefined undefined Red,Blue,

Green ["Red", "Blue", "Green"] Red, Blue, Green 2 ", " undefined Red,Blue,
Green ["Red", "Blue", "Green"] Red, Blue, Green 3 ";" undefined Red;Blue;
Green ["Red", "Blue", "Green"] Red;Blue;Green 4 "; " undefined Red;Blue;
Green ["Red", "Blue", "Green"] Red; Blue; Green 5 ";" "; " Red;Blue;
Green ["Red", "Blue", "Green"] Red; Blue; Green 6 "/[,;]\s_/" "; " Red;
Blue, Green ["Red", "Blue", "Green"] Red; Blue; Green 7 "/[,;]\s_/"
undefined Red; Blue, Green ["Red", "Blue", "Green"] Compile Error 8 "/
or /" " or " House or Door or Car ["House", "Door", "Car"] House or Door
or Car 9 "/[,;]\s*/" "-" Red; Blue, Green ["Red", "Blue", "Green"]
Red-Blue-Green 10 ";" "," Red; Blue; Green ["Red", "Blue", "Green"] Compile
Error

This logic is:

  • If the separator and join string are undefined, the separator is ","
    and the join string is ", " (1)
  • If the separator is specified and the join string is undefined, the
    join string is the same as the separator (2,3,4)
  • If the separator has whitespace, it will be trimmed (4)
  • If the join string is specified and trim(separator) ==
    trim(joinstring), the untrimmed join string is used to render the view (5)
  • If a regular expression is used then a join string must be specified
    (6,7)
  • If you want the separator to consider whitespace, use a regular
    expression + join string (8)
  • There is no protection if you decide to use an inconsistent join
    string with your regular expression (9)
  • If both the separator and join string are specified and
    trim(separator) != trim(join) then an error is thrown (10)

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.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2561#issuecomment-17404536
.

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Jul 10, 2013

I think we have two options here:

1/ simple

  • drop regexp support
  • use the current separator for splitting but treat all separator whitespace as insignificant
  • use the current separator for joining but treat all separator whitespace as significant

2/ complex

  • add the joiner and make it work in all scenarios described in the table above
  • I would skip throwing any kind of errors for cases 10 and 7
  • I would focus on keeping the code small

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.

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Jul 10, 2013

Personally I would go with 1/ as well. I would be even tempted to consider removing it from the core....

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Jul 11, 2013

ok, let's go with 1/ can we get this PR updated to reflect that, please?

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Jul 31, 2013

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 mhevery closed this Jul 31, 2013

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Jul 31, 2013

@mhevery - So are we going to implement the simple solution? At the moment the joining nature of ng-list is very broken.

purcell added a commit to purcell/angular.js that referenced this pull request Oct 9, 2013

fix(input): make ngList honor custom separators
Remove support for regexp separators, and correctly use custom separator
when formatting values into the control.

See angular#4008 and angular#2561.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jul 16, 2014

fix(ngList): use custom separators for re-joining list items
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

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jul 17, 2014

fix(ngList): use custom separators for re-joining list items
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

petebacondarwin added a commit that referenced this pull request Jul 17, 2014

fix(ngList): use custom separators for re-joining list items
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.