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

[Select] new itemDisabled prop #2580

Merged
merged 7 commits into from
Jun 11, 2018
Merged

[Select] new itemDisabled prop #2580

merged 7 commits into from
Jun 11, 2018

Conversation

giladgray
Copy link
Contributor

Changes proposed in this pull request:

  • new itemDisabled: keyof T | (T => boolean) prop allows disabling items in the list!
    • provide callback or prop name shorthand (a la lodash)
  • no tests yet as the QueryList suite just has placeholders for relevant tests

@giladgray giladgray requested review from invliD and llorca June 8, 2018 20:37
@blueprint-bot
Copy link

SelectExample option to disable films before year 2000

Preview: documentation | landing | table

initialContent={initialContent}
noResults={<MenuItem disabled={true} text="No results." />}
onItemSelect={this.handleValueChange}
popoverProps={{ minimal }}
>
<Button rightIcon="caret-down" text={film ? film.title : "(No selection)"} disabled={disabled} />
<Button
icon="film"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't review the code, but works great and looks great 👍

@blueprint-bot
Copy link

moveActiveIndex => getNextActiveItem returns T

Preview: documentation | landing | table

@blueprint-bot
Copy link

required index param

Preview: documentation | landing | table

@@ -106,4 +120,6 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
this.setState(state => ({ ...state, [prop]: checked }));
};
}

private isItemDisabled = (film: IFilm) => this.state.disableItems && film.year < 2000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works because no films were ever produced prior to Jan 1, 1990 😜

return undefined;
} else if (this.isItemDisabled(filteredItems[nextActiveIndex], nextActiveIndex)) {
// keep on moving in given direction if this item is disabled.
return this.getNextActiveItem(direction, nextActiveIndex, startIndex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems a little weird to use recursion on an unbounded list when iterating would suffice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i found recursion to be the simplest code change here (tho i then went ham on the refactor 🤷‍♂️)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will refactor to use a loop

@themadcreator
Copy link
Contributor

I'll note that if you select shawshank then disable items from 1990's, it remains selected. I don't think that's necessarily a bug, but it does mean that if you change the selection, you can't revert to shawshank.

@giladgray
Copy link
Contributor Author

that's an artifact of the example implementation. selection is purely controlled by the parent.

half the line count!
@blueprint-bot
Copy link

break recursion with a do/while

Preview: documentation | landing | table

@giladgray giladgray merged commit 833ea83 into develop Jun 11, 2018
@giladgray giladgray deleted the gg/item-disabled branch June 11, 2018 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants