-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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] un/controlled activeItem & query! #2747
Conversation
move required props from IQueryListProps to optional in IListItemsProps! add handleQueryChange to renderer props. move activeItem/query state into IQueryListState
… enabled first item
Merge branch 'develop' of github.com:palantir/blueprint into gg/reset-on-selectPreview: documentation | landing | table |
revert Suggest (followup PR with more extensive changes)Preview: documentation | landing | table |
/>, | ||
), | ||
); | ||
// TODO: Suggest does not use the new APIs yet as it requires more extensive refactors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in #2748
private renderQueryList = (listProps: IQueryListRendererProps<T>) => { | ||
const { inputProps = {}, isOpen, overlayProps = {} } = this.props; | ||
const { handleKeyDown, handleKeyUp } = listProps; | ||
const handlers = isOpen && !this.isQueryEmpty() ? { onKeyDown: handleKeyDown, onKeyUp: handleKeyUp } : {}; | ||
const handlers = isOpen && listProps.query.length > 0 ? { onKeyDown: handleKeyDown, onKeyUp: handleKeyUp } : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
query is optional on props, maybe check null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listProps.query
is always defined as it comes from QueryList
*/ | ||
filteredItems: T[]; | ||
|
||
export interface IQueryListRendererProps<T> extends IQueryListState<T>, IProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this props extends state?
filteredItems: T[]; | ||
|
||
/** The current query string. */ | ||
query: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this resolved with query from props? is this just to allow control/uncontrolled variants?
this.setState({ filteredItems: getFilteredItems(this.props) }); | ||
public componentWillReceiveProps(nextProps: IQueryListProps<T>) { | ||
if (nextProps.activeItem != null) { | ||
this.setState({ activeItem: nextProps.activeItem }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any way to DEactivate items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allow explicit null
for "no active item" ?
@@ -189,6 +182,19 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList | |||
} | |||
} | |||
|
|||
// TODO resetActiveItem = this.props.resetOnQuery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have plans to do this before merge or just leaving this in there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a followup, separate feature. not before merging.
// TODO: this will be more complex with un/controlled support (next PR) | ||
Utils.safeInvoke(this.props.onActiveItemChange, item); | ||
private setActiveItem(activeItem: T | undefined) { | ||
if (this.props.activeItem == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only set state if it's null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if props is null then we're uncontrolled, so update state. otherwise inform the user and let them update props.
@@ -137,15 +109,18 @@ export class Select<T> extends React.PureComponent<ISelectProps<T>, ISelectState | |||
// not using defaultProps cuz they're hard to type with generics (can't use <T> on static members) | |||
const { filterable = true, disabled = false, inputProps = {}, popoverProps = {} } = this.props; | |||
|
|||
const clearButton = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kind of prefer the .maybe*()
methods
@@ -51,32 +51,15 @@ export interface IOmnibarProps<T> extends IListItemsProps<T> { | |||
|
|||
/** The query string. */ | |||
query?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved after in person review review
* refactor Suggest to use QueryList's activeItem and query also remove isTyping state, which fixes double render. when open, selectedItem appears in placeholder instead of input value. this is a nicer experience and works great with resetOnSelect. * add resetOnSelect switch to Suggest example * revert disabled tests * fix esc/tab tests these keys used to clear the selection, now they just cancel a selection in progress and keep the previous selection state. * default placeholder
- explicit null for "no active item" - undefined optional prop puts it in uncontrolled mode
[Suggest] refactor to support new QueryList state (#2748)Preview: documentation | landing | table |
maybeRenderClearButtonPreview: documentation | landing | table |
+ This is more of a breaking change than the package version would indicate - a "significant refactor" as per palantir/blueprint#2747. + Breaks our current wrapping of the suggest API - need to investigate and update properly. + Filed #558 to track.
Significant refactor to
select
components in which most state is moved intoQueryList
enabling controlled and uncontrolled usage ofquery
andactiveItem
(keyboard selection state).query
/onQueryChange
,activeItem
/onActiveItemChange
props supported in all 4 components, implemented inQueryList
resetOnSelect
has also been made a "common" prop, supported natively inQueryList
QueryList
(like a few trackisOpen
for their popover)Reviewers should focus on:
Suggest
in here because it requires more work than the others. See [Suggest] refactor to support new QueryList state #2748