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

feat(ngList): support whitespace control through ngTrim #8216

Closed
wants to merge 3 commits into from

Conversation

petebacondarwin
Copy link
Contributor

This PR sits on top of #4344 to make ngList consistent with standard use of ngTrim and useful in scenarios where you want to split on whitespace strings.

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8216)

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!

@petebacondarwin
Copy link
Contributor Author

@IgorMinar, @purcell, @tedjp, @hnordt - can you take a look at this ngTrim feature?

@caitp
Copy link
Contributor

caitp commented Jul 16, 2014

okay, having read this, I think we're saying that ngTrim now affects the way ngList separators work, by making whitespace before/after the separator optional (or not)?

The feature seems okay, but reading the markup in the tests, it's not totally communicating the intent of this behaviour to me. Either way, I guess that makes sense.

@petebacondarwin
Copy link
Contributor Author

Yes that's the idea. I'm not sure how to make the tests clearer. @caitp any ideas?

@purcell
Copy link
Contributor

purcell commented Jul 16, 2014

@petebacondarwin Looks sound to me.

@caitp
Copy link
Contributor

caitp commented Jul 16, 2014

@petebacondarwin I don't think the tests are the problem, to me I just think that using the feature declaratively is not really self-documenting. It's not clear that ng-trim="false" means "don't trim the whitespace separator". ng-trim-separator would be clearer, but we don't want to add a new attribute, so it's fine

@IgorMinar
Copy link
Contributor

this looks good to me.

@caitp from the user perspective ngTrim is applied onto the individual parts in the list and not on the separator, so ng-trim-separator doesn't make things any better.

@caitp
Copy link
Contributor

caitp commented Jul 16, 2014

@IgorMinar not if you read the tests, that's not actually what's happening. I mean, in practice, it is what's happening, but it's not how you'd describe the end result --- so the end result doesn't seem to make sense based on the input. I think a new attribute would make more sense, but I don't think we want to add a new attribute, so it looks good to me. (It's just, confusing, I think this will result in code which is actually more confusing than it should be --- but maybe that's better than adding yet another API to make it clearer)

@petebacondarwin
Copy link
Contributor Author

@IgorMinar

Yes, I was trying to minimize changes to the API. One of the goals of this PR was to fix up ngList without trying to add too much complexity to the directive [https://github.com//pull/2561#issuecomment-20843976].

Much of this has been discussed before in #2561 and #4008.

The standard meaning of ngTrim is to trim (or not) whitespace from the model value. In this case I have implemented ngTrim in ngList so that it trims each list item, as you say. But in addition, we want to be able support a couple of scenarios easily:

  1. trim the separator but not the joiner: e.g. split on "," but join on ", ".
  2. split on a separator that is only white space: e.g. "\n"

To accomplish this I have deputized the ngTrim attribute, in such a way that (to me) seems intuitive. In simple terms, if you specify ngTrim="false" then no trimming of anything (separators nor values) will occur. I outline this in the update to the docs:

The behaviour of the directive is affected by the use of the ngTrim attribute.

  • If ngTrim is set to "false" then whitespace around both the separator and each
    list item is respected. This implies that the user of the directive is responsible for
    dealing with whitespace but also allows you to use whitespace as a delimiter, such as a
    tab or newline character.
  • Otherwise whitespace around the delimiter is ignored when splitting (although it is respected
    when joining the list items back together) and whitespace around each list item is stripped
    before it is added to the model.

Does this seem reasonable too? I think @caitp is not 100% happy but is being pragmatic and thinks it is a fair compromise.

purcell and others added 3 commits July 17, 2014 20:13
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
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
Copy link
Contributor Author

Landed as 8d18d20

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.

5 participants