feat(chips): allow to specify a maximum of chips that can be added through user input #6897

devversion · 2016-01-28T15:43:38Z

The current behavior on chips limit reaching is that there will happen nothing (no input clear).
Any suggestions for the behavior?

Fixes #6864

devversion · 2016-01-28T15:45:48Z

src/components/chips/js/chipsController.js

 */
 MdChipsCtrl.prototype.appendChip = function(newChip) {
+  // Don't append the chip if the maxmimum is reached
+  // Return true, to show that the limit is reached
+  if (this.items.length >= this.maxChips) return true;


I can swap the return values, it would make more sense to return false if the chip can't be added, but then I need to return false if the chip is a duplicate too... and in this case it should clear the buffer. (so I think the current solution is the most simple)

--- Improved.

ThomasBurleson · 2016-01-31T14:05:37Z

We should support ng-messages with custom errors when the limit is reached. (much like input with errors).

ThomasBurleson · 2016-01-31T14:06:36Z

Without ngMessages support, this feature creates more issues than it solves. Removed from the merge-queue until feature is finished.

devversion · 2016-01-31T17:51:42Z

@ThomasBurleson All done 👍 Would be great if you review again.

devversion · 2016-01-31T18:06:33Z

src/components/chips/js/chipsDirective.js

@@ -67,6 +67,9 @@
   *    displayed when there is at least on item in the list
   * @param {boolean=} readonly Disables list manipulation (deleting or adding list items), hiding
   *    the input and delete buttons
+   * @param {number=} md-max-chips The maximum number of chips allowed to add through user input.
+   *    <br/><br/>The validation property `md-max-chips` can be used when the max chips
+   *    amount is reached.


Please review here @ThomasBurleson

ThomasBurleson · 2016-02-04T15:55:17Z

The demo is broken and does not work: entire a 4th chip and the error message shows.
@devversion - did you test this ?

devversion · 2016-02-04T15:56:53Z

@ThomasBurleson - I was sure everything works (testing etc). Will check it now!

devversion · 2016-02-04T15:57:46Z

@ThomasBurleson - Update 😄 - Forgot adding ngMessages as dependency.

devversion · 2016-02-04T16:00:55Z

@ThomasBurleson - Now fixed!

ThomasBurleson · 2016-02-04T16:16:16Z

@devversion:

what about the error text color?
Is the error message too close to the chip bottom border?

devversion · 2016-02-04T16:18:23Z

I don't know, I thought I should just add ngMessages support - but I can create a custom styling just for the styling? Support for ngMessages for the complete chips will be another issue / PR, (would like to that, if needed 😄 )

ThomasBurleson · 2016-02-04T16:18:44Z

Also this logic appears wrong:

MdChipsCtrl.prototype.validateModel = function() {
  var isMaximumReached = !(this.maxChips && this.items.length >= this.maxChips);
  this.ngModelCtrl.$setValidity('md-max-chips', isMaximumReached);
};

And this.items.length is a number while this.maxChips is === "5".

Please use this code:

MdChipsCtrl.prototype.hasMaxChips = function() {
  this.maxChips = (angular.isString(this.maxChips) ) ? parseInt(this.maxChips,10) || 0 :
                  (angular.isUndefined(this.maxChips) ) ? 0 : this.maxChips;

  return (this.maxChips > 0) && (this.items.length >= this.maxChips);
};

ThomasBurleson · 2016-02-04T16:20:22Z

full Support for ngMessages is overkill for this PR. Yet if we use ngMessages to show an error, then that message should be colored accordingly IMO.

devversion · 2016-02-04T16:20:44Z

this.maxChips won't be a string, it will be interpolated from the scope, so it will be an integer? Or you mean, when the user specifies a string? (like md-max-chips="hello 1")?

ThomasBurleson · 2016-02-04T16:27:19Z

It is a string... inspect in the dev console.

devversion · 2016-02-04T16:33:54Z

@ThomasBurleson - Sorry wrong approach.. Updated it, added style to the custom ngMessages in the demo? and used your string validation for maxChips - (working on the CI)

ThomasBurleson · 2016-02-04T16:40:22Z

@devversion - you need to thoroughly test each PR before submitting. Otherwise the impacts could be huge.

…rough user input The current behavior, when the limit is reached, that there will happen nothing (no input clear). But this behavior should be tested for the UX Fixes angular#6864

ThomasBurleson · 2016-02-04T16:46:20Z

src/components/chips/chips.spec.js

@@ -409,6 +409,71 @@ describe('<md-chips>', function() {
        }));
      });

+      describe('md-max-chips', function() {


@devversion - are these passing for you ?

Now Passing (just said in the comment before 😄, so please don't be to harsh).

Updated the check to ->

MdChipsCtrl.prototype.hasMaxChips = function() { if (angular.isString(this.maxChips)) this.maxChips = parseInt(this.maxChips, 10) || 0; return this.maxChips > 0 && this.items.length >= this.maxChips; };

If undefined the it will return false, because undefined > 0 will be false.

devversion · 2016-02-04T16:46:32Z

@ThomasBurleson - Yes, next time I will test it more thoroughly. Just wasn't sure about ngMessages, but in my case everything worked perfect until the ngMessages support was requested. So next time I will check more thoroughly when updating a PR.

…dded through user input Current UX provides no indication of when max chips has been reached. * use ngMessages to show information at max limit condition. Fixes angular#6864. Closes angular#6897.

…dded through user input Current UX provides no indication of when max chips has been reached. * use ngMessages to show information at max limit condition. Fixes #6864. Closes #6897. # Conflicts: # src/components/chips/chips.spec.js # src/components/chips/js/chipsController.js

devversion reviewed Jan 28, 2016
View reviewed changes

devversion force-pushed the feat/chips-max-limit branch 2 times, most recently from a97d629 to f027e63 Compare January 28, 2016 16:38

ThomasBurleson added the type: enhancement label Jan 30, 2016

ThomasBurleson added this to the 1.0.5 milestone Jan 30, 2016

ThomasBurleson self-assigned this Jan 30, 2016

ThomasBurleson added the pr: merge ready This PR is ready for a caretaker to review label Jan 30, 2016

ThomasBurleson added needs: work and removed pr: merge ready This PR is ready for a caretaker to review labels Jan 31, 2016

ThomasBurleson modified the milestones: Backlog, 1.0.5 Jan 31, 2016

devversion force-pushed the feat/chips-max-limit branch 3 times, most recently from 2654e3a to 36dc394 Compare January 31, 2016 17:49

devversion force-pushed the feat/chips-max-limit branch from 36dc394 to c30042f Compare January 31, 2016 18:06

devversion reviewed Jan 31, 2016
View reviewed changes

ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: work labels Feb 4, 2016

ThomasBurleson modified the milestones: 1.0.5, Backlog Feb 4, 2016

ThomasBurleson added needs: work and removed pr: merge ready This PR is ready for a caretaker to review labels Feb 4, 2016

devversion force-pushed the feat/chips-max-limit branch from f0c7675 to b214c37 Compare February 4, 2016 16:00

devversion force-pushed the feat/chips-max-limit branch from b214c37 to bf4d1e3 Compare February 4, 2016 16:32

ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: work labels Feb 4, 2016

feat(chips): allow to specify a maximum of chips that can be added th…

0cd886f

…rough user input The current behavior, when the limit is reached, that there will happen nothing (no input clear). But this behavior should be tested for the UX Fixes angular#6864

devversion force-pushed the feat/chips-max-limit branch from bf4d1e3 to 0cd886f Compare February 4, 2016 16:44

ThomasBurleson reviewed Feb 4, 2016
View reviewed changes

ThomasBurleson closed this in 03caf58 Feb 4, 2016

devversion deleted the feat/chips-max-limit branch April 21, 2016 12:06

feat(chips): allow to specify a maximum of chips that can be added through user input #6897

feat(chips): allow to specify a maximum of chips that can be added through user input #6897

Uh oh!

Conversation

devversion commented Jan 28, 2016

Uh oh!

devversion Jan 28, 2016

Choose a reason for hiding this comment

Uh oh!

devversion Jan 28, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasBurleson commented Jan 31, 2016

Uh oh!

ThomasBurleson commented Jan 31, 2016

Uh oh!

devversion commented Jan 31, 2016

Uh oh!

devversion Jan 31, 2016

Choose a reason for hiding this comment

Uh oh!

ThomasBurleson commented Feb 4, 2016

Uh oh!

devversion commented Feb 4, 2016

Uh oh!

devversion commented Feb 4, 2016

Uh oh!

devversion commented Feb 4, 2016

Uh oh!

ThomasBurleson commented Feb 4, 2016

Uh oh!

devversion commented Feb 4, 2016

Uh oh!

ThomasBurleson commented Feb 4, 2016

Uh oh!

ThomasBurleson commented Feb 4, 2016

Uh oh!

devversion commented Feb 4, 2016

Uh oh!

ThomasBurleson commented Feb 4, 2016

Uh oh!

devversion commented Feb 4, 2016

Uh oh!

ThomasBurleson commented Feb 4, 2016

Uh oh!

ThomasBurleson Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

devversion Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

devversion commented Feb 4, 2016

Uh oh!

Uh oh!