Implemented BasePicker itemLimit #2553

ohritz · 2017-08-19T09:34:29Z

Pull request checklist

Addresses an existing issue: Fixes Pickers: Specify maximum number of items to select #1304
Include a change request file using $ npm run change

Description of changes

Added a prop "itemLimit" on the basePicker
logic to not render the autofill "input" if the itemLimit is defined and equal to internal item count
Added test for previous logic.
Added prop to the TagPicker in the example.

msftclas · 2017-08-19T09:34:35Z

@ohritz,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

dzearing · 2017-08-20T20:15:54Z

packages/office-ui-fabric-react/src/components/pickers/BasePicker.Props.ts

@@ -93,6 +93,11 @@ export interface IBasePickerProps<T> extends React.Props<any> {
   * @default false


Please add a change request json file. Instructions:

make sure you have an upstream remote to officedev url (I call mine "upstream")

npm run change -- --b upstream/master

This change is a "minor" package bump.

For comment, these show up in release notes. Please follow the convention ComponentName: change description.

BasePicker: added itemLimit property (etc).

ahh :) sorry about that.. done that now. Thanks.

dzearing · 2017-08-21T15:47:02Z

packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx

+  protected okToAddMore(): boolean {
+    const { items } = this.state;
+    const { itemLimit } = this.props;
+    if (typeof itemLimit !== 'undefined' && items.length === itemLimit) {


minor suggestion; this can be shortened to itemLimit !== undefined. AFAIK you only really need to use typeof when you are checking if a global variable like document exists (to prevent throwing) or you really care about the primitive type.

Also, you can further shorten by not even comparing undefined and just returning the length comparison. There really isn't a need to check itemLimit undefined unless you expect items.length to also potentially be undefined.

return ( items.length === itemLimit );

true.. will change..

just a thought.. performance wise...
checking for undefined on itemLimit would exit early and skip checking the length of items. a micro optimization granted, but is it worth keeping in?

I tried the shortest version.

return items.length < itemLimit;

and it seems to break existing functionality, when running the tests, as itemLimit is not passed in in most cases.
I think we should go with

return itemLimit === undefined || items.length < itemLimit;

so, when its undefined, its canAddMore() === true
and when its defined, items.length has to be less than itemLimit to return true..

dzearing · 2017-08-21T15:49:42Z

packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx

@@ -234,6 +243,9 @@ export class BasePicker<T, P extends IBasePickerProps<T>> extends BaseComponent<
      if (newEl) {
        this.focusZone.focusElement(newEl);
      }
+    } else if (!this.okToAddMore()) {
+      (items[items.length - 1] as IPickerItemProps<T>).selected = true;


if items.length is 0, you would null ref here.

hmm.. not so sure we would get into this branch if items.length is 0 unless itemLimit is set to 0, and then I'm not sure the set focus would be triggered..
Will write some tests and correct accordingly..

What if we log warning when usrer sets itemLimit to 0 and recommend using disabled instead? And default to 1 if it is set to 0?

I wrote two tests and it seems like it should be ok, what do you think?

dzearing · 2017-08-21T15:50:20Z

packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx

@@ -180,6 +180,15 @@ export class BasePicker<T, P extends IBasePickerProps<T>> extends BaseComponent<
    );
  }

+  protected okToAddMore(): boolean {


minor suggestion; canAddItems may be a better name. ("more" of what? "ok to" is also inconsistent)

joschect · 2017-08-21T18:39:11Z

packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx

@@ -156,7 +156,7 @@ export class BasePicker<T, P extends IBasePickerProps<T>> extends BaseComponent<
          <SelectionZone selection={ this.selection } selectionMode={ SelectionMode.multiple }>
            <div className={ css('ms-BasePicker-text', styles.pickerText) } role={ 'list' }>
              { this.renderItems() }
-              <BaseAutoFill
+              { this.okToAddMore() && (<BaseAutoFill
                { ...inputProps as any }


One thing I'm not quite sure about is where focus will wind up. Normally when you select something, focus is given back to the input, in this case the input won't exist any more. I think focus and selection should be given to the last item selected in the list. I think you'll need to put the check in componentwillupdate

That's what I'm trying to achieve on line 246-248, seems to me that resetFocus() handles focus being set correctly and is called from several places. you think I should put some check in in componentwillupdate as well?

Ah! I totally missed that, should be good.

ohritz · 2017-08-22T17:57:21Z

I wrote a test to verify the last item picked was in focus when itemLimit was reached.. and discovered a flaw in my implementation.. Testing in the browser it seems that its given focus.. but pressing backspace or del at that moment causes an error, as the onBackspace method relies on the input being around.

Im wondering if I should change the logic in the onBackspace, or if its a potential rabbit hole. and go with disabling the input instead on canAddItems() === false and leaving it in the dom?

ohritz · 2017-08-22T21:55:19Z

the test I wrote to validate that the last picked item had focus led me down a path of minor changes which are in commit cc8ee04, everything seems to work fine in the browser, but I could not get the test to function.. trying to find the focused element and comparing it to expected value does not seem to work.

seems to be something in phantomjs.
see ariya/phantomjs#11432

I think I can write a derived picker where I can pass in a render function and use it as a spy to pull out the element being set as focused?.

Picker itemLimit implemented

a2c06f4

msftclas added the cla-signed label Aug 19, 2017

Merge branch 'master' into master

d152028

dzearing requested a review from joschect August 20, 2017 20:13

dzearing reviewed Aug 20, 2017

View reviewed changes

ohritz and others added 6 commits August 20, 2017 23:13

added change request

ad378b3

Merge branch 'master' of github.com:OfficeDev/office-ui-fabric-react

43ee62b

removed leftover variable assignment from previous iteration

7ca09a1

removed debug remnant..

353b574

added jsDoc defualt value for itemLimit Prop

b9db12d

Merge branch 'master' into master

602792e

dzearing reviewed Aug 21, 2017

View reviewed changes

joschect reviewed Aug 21, 2017

View reviewed changes

ohritz and others added 5 commits August 21, 2017 23:04

simplified logic and changed function name to be more consistent

a133687

added more tests for itemLimit

a7704ee

Merge branch 'master' of github.com:ohritz/office-ui-fabric-react

42907ed

Merge branch 'master' of github.com:OfficeDev/office-ui-fabric-react

29dc456

Merge branch 'master' into master

5edf8f4

ohritz added 3 commits August 22, 2017 23:28

Merge branch 'master' of github.com:OfficeDev/office-ui-fabric-react

3125459

fixes in focus handling when itemLimit set. (needs test)

cc8ee04

Merge branch 'master' of github.com:ohritz/office-ui-fabric-react

816a362

dzearing approved these changes Aug 24, 2017

View reviewed changes

joschect approved these changes Aug 24, 2017

View reviewed changes

dzearing merged commit cd5b68b into microsoft:master Aug 24, 2017

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented BasePicker itemLimit #2553

Implemented BasePicker itemLimit #2553

ohritz commented Aug 19, 2017 •

edited

Loading

msftclas commented Aug 19, 2017

dzearing Aug 20, 2017 •

edited

Loading

ohritz Aug 20, 2017

dzearing Aug 21, 2017

dzearing Aug 21, 2017

ohritz Aug 21, 2017

ohritz Aug 21, 2017

ohritz Aug 21, 2017 •

edited

Loading

dzearing Aug 21, 2017

ohritz Aug 21, 2017 •

edited

Loading

ohritz Aug 21, 2017

dzearing Aug 21, 2017

ohritz Aug 21, 2017

joschect Aug 21, 2017

ohritz Aug 21, 2017

joschect Aug 21, 2017

ohritz commented Aug 22, 2017

ohritz commented Aug 22, 2017 •

edited

Loading

		@@ -93,6 +93,11 @@ export interface IBasePickerProps<T> extends React.Props<any> {
		* @default false

Implemented BasePicker itemLimit #2553

Implemented BasePicker itemLimit #2553

Conversation

ohritz commented Aug 19, 2017 • edited Loading

Pull request checklist

Description of changes

msftclas commented Aug 19, 2017

dzearing Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohritz Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohritz Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohritz commented Aug 22, 2017

ohritz commented Aug 22, 2017 • edited Loading

ohritz commented Aug 19, 2017 •

edited

Loading

dzearing Aug 20, 2017 •

edited

Loading

ohritz Aug 21, 2017 •

edited

Loading

ohritz Aug 21, 2017 •

edited

Loading

ohritz commented Aug 22, 2017 •

edited

Loading