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

fix(input): make ngList honor custom separators #4344

Closed
wants to merge 1 commit into from

Conversation

purcell
Copy link
Contributor

@purcell purcell commented Oct 9, 2013

Remove support for regexp separators, and correctly use custom separator when formatting values into the control.

Follows @IgorMinar's suggestions from #2561, as suggested by @petebacondarwin in #4008.

The existing "should allow custom separator" test was incomplete, since it did not check that the custom separator was used to format the values in the control.

Notes:

  1. The behaviour is still not perfect, because when using a custom separator containing whitespace, e.g. " | ", the spaces should be rendered into the control. Even with my changes, this is not the case, because the attr.ngList value has already been trimmed prior to being passed to the directive. I have omitted the failing test for this case. How would we handle this?
  2. The whitespace behaviour should arguably be affected by the value of ng-trim where present: I'd suggest that when it is present, the values between the separators should not be trimmed.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@purcell
Copy link
Contributor Author

purcell commented Oct 9, 2013

I have previously signed a CLA, for #4207, but since that has apparently not been reviewed, I'll go ahead and sign another CLA. (Name: Stephen Purcell / Steve Purcell)

The mary-poppins bot doesn't seem to detect the valid commit message format, but I have the pre-commit hook installed locally and passing.

Remove support for regexp separators, and correctly use custom separator
when formatting values into the control.

See angular#4008 and angular#2561.
@ghost ghost assigned IgorMinar Nov 22, 2013
@IgorMinar
Copy link
Contributor

LGTM, but since this requires a breaking change, I'm going to triage it for 1.3.x. It should be one of the first PRs to make it in.

thanks for all the work!

@tedjp
Copy link

tedjp commented Feb 27, 2014

Is there a way for this to split on newline for <textarea>s? I don't think "\n" works.

@hnordt
Copy link

hnordt commented Jul 1, 2014

+1

@petebacondarwin
Copy link
Contributor

@tedjp - that is a good point. We should somehow support that without it becoming too complex

@petebacondarwin
Copy link
Contributor

@tedjp - perhaps we could use &#10;?

@petebacondarwin
Copy link
Contributor

@petebacondarwin
Copy link
Contributor

OK, so to break on newlines we need to be able to get the whitespace from the ngList attribute.
This is possible using element.attr(attr.$attr.ngList).

Then we have the other question of whether to trim whitespace from the the list items. At the moment we always trim but if we are not going to trim the whitespace from the separator string then we might have a problem.

For instance, how should ng-list="|" and ng-list=" | " be treated both on splitting and joining? One could assume that for splitting a|b|c should be split the same by both into ['a', 'b', 'c'] but that when joining one would return a|b|c and the other a | b | c. Equally, a | b | c could be split into ['a', 'b', 'c'] or ['a ', ' b ', ' c'] depending on how we treat whitespace.

@petebacondarwin
Copy link
Contributor

How about this?

If ngTrim is undefined or true (default):

  • split the value using the trimmed ngList value
  • trim the whitespace from each item after it has been split
  • join the items back using the untrimmed ngList value

If ngTrim is false (user is taking control of the whitespace)

  • split the value using the untrimmed ngList value
  • do not trim whitespace from each item after it has been split
  • join the items back using the untrimmed ngList value

@petebacondarwin petebacondarwin modified the milestones: 1.3.0-beta.16, 1.3.0 Jul 15, 2014
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Jul 16, 2014
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 16, 2014
With the removal of regular expression support `ngList` no longer supported
splitting on newlines (and other pure whitespace splitters).

This change allows the application developer to specify whether whitespace
should be respected or trimmed by using the `ngTrim` attribute. This also
makes `ngList` consistent with the standard use of `ngTrim` in input directives
in general.

Related To: angular#4344
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request Jul 16, 2014
With the removal of regular expression support `ngList` no longer supported
splitting on newlines (and other pure whitespace splitters).

This change allows the application developer to specify whether whitespace
should be respected or trimmed by using the `ngTrim` attribute. This also
makes `ngList` consistent with the standard use of `ngTrim` in input directives
in general.

Related To: angular#4344
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Jul 17, 2014
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
With the removal of regular expression support `ngList` no longer supported
splitting on newlines (and other pure whitespace splitters).

This change allows the application developer to specify whether whitespace
should be respected or trimmed by using the `ngTrim` attribute. This also
makes `ngList` consistent with the standard use of `ngTrim` in input directives
in general.

Related To: angular#4344
petebacondarwin added a commit that referenced this pull request Jul 17, 2014
With the removal of regular expression support `ngList` no longer supported
splitting on newlines (and other pure whitespace splitters).

This change allows the application developer to specify whether whitespace
should be respected or trimmed by using the `ngTrim` attribute. This also
makes `ngList` consistent with the standard use of `ngTrim` in input directives
in general.

Related To: #4344
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants