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] Add resetOnQuery prop #2894

Merged
merged 4 commits into from
Sep 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface ISelectExampleState {
hasInitialContent: boolean;
minimal: boolean;
resetOnClose: boolean;
resetOnQuery: boolean;
resetOnSelect: boolean;
disableItems: boolean;
disabled: boolean;
Expand All @@ -33,6 +34,7 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
hasInitialContent: false,
minimal: false,
resetOnClose: false,
resetOnQuery: true,
resetOnSelect: false,
};

Expand All @@ -42,6 +44,7 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
private handleItemDisabledChange = this.handleSwitchChange("disableItems");
private handleMinimalChange = this.handleSwitchChange("minimal");
private handleResetOnCloseChange = this.handleSwitchChange("resetOnClose");
private handleResetOnQueryChange = this.handleSwitchChange("resetOnQuery");
private handleResetOnSelectChange = this.handleSwitchChange("resetOnSelect");

public render() {
Expand Down Expand Up @@ -87,6 +90,11 @@ export class SelectExample extends React.PureComponent<IExampleProps, ISelectExa
checked={this.state.resetOnClose}
onChange={this.handleResetOnCloseChange}
/>
<Switch
label="Reset on query"
checked={this.state.resetOnQuery}
onChange={this.handleResetOnQueryChange}
/>
<Switch
label="Reset on select"
checked={this.state.resetOnSelect}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface ISuggestExampleState {
minimal: boolean;
openOnKeyDown: boolean;
resetOnSelect: boolean;
resetOnQuery: boolean;
}

export class SuggestExample extends React.PureComponent<IExampleProps, ISuggestExampleState> {
Expand All @@ -27,12 +28,14 @@ export class SuggestExample extends React.PureComponent<IExampleProps, ISuggestE
film: TOP_100_FILMS[0],
minimal: true,
openOnKeyDown: false,
resetOnQuery: true,
resetOnSelect: false,
};

private handleCloseOnSelectChange = this.handleSwitchChange("closeOnSelect");
private handleOpenOnKeyDownChange = this.handleSwitchChange("openOnKeyDown");
private handleMinimalChange = this.handleSwitchChange("minimal");
private handleResetOnQueryChange = this.handleSwitchChange("resetOnQuery");
private handleResetOnSelectChange = this.handleSwitchChange("resetOnSelect");

public render() {
Expand Down Expand Up @@ -65,6 +68,11 @@ export class SuggestExample extends React.PureComponent<IExampleProps, ISuggestE
checked={this.state.openOnKeyDown}
onChange={this.handleOpenOnKeyDownChange}
/>
<Switch
label="Reset on query"
checked={this.state.resetOnQuery}
onChange={this.handleResetOnQueryChange}
/>
<Switch
label="Reset on select"
checked={this.state.resetOnSelect}
Expand Down
7 changes: 7 additions & 0 deletions packages/select/src/common/listItemsProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ export interface IListItemsProps<T> extends IProps {
*/
onQueryChange?: (query: string, event?: React.ChangeEvent<HTMLInputElement>) => void;

/**
* Whether the active item should be reset to the first matching item every
* time the query changes.
* @default true
*/
resetOnQuery?: boolean;

/**
* Whether the querying state should be reset to initial when an item is
* selected (immediately before `onItemSelect` is invoked). The query will
Copy link
Contributor

Choose a reason for hiding this comment

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

Whether the active item should be reset to the first matching item when an item is selected.
The query will also be reset to the empty string.

and please make this same change in resetOnClose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow -- you want that to replace the docs for resetOnSelect (in listItemProps.ts) & resetOnClose (in select.tsx)?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the goal. i can do it if you're not comfortable

Expand Down
7 changes: 5 additions & 2 deletions packages/select/src/components/query-list/queryList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ export interface IQueryListState<T> {
export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryListState<T>> {
public static displayName = `${DISPLAYNAME_PREFIX}.QueryList`;

public static defaultProps = {
resetOnQuery: true,
};

public static ofType<T>() {
return QueryList as new (props: IQueryListProps<T>) => QueryList<T>;
}
Expand Down Expand Up @@ -182,8 +186,7 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList
}
}

// TODO resetActiveItem = this.props.resetOnQuery
public setQuery(query: string, resetActiveItem = false) {
public setQuery(query: string, resetActiveItem = this.props.resetOnQuery) {
if (query !== this.state.query) {
Utils.safeInvoke(this.props.onQueryChange, query);
}
Expand Down
26 changes: 24 additions & 2 deletions packages/select/test/selectComponentSuite.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function selectComponentSuite<P extends IListItemsProps<IFilm>, S>(

describe("common behavior", () => {
it("itemRenderer is called for each child", () => {
const wrapper = render(testProps);
const wrapper = render({ ...testProps, resetOnQuery: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this necessary? does setting the query prop cause an extra re-render?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it re-renders the items

Copy link
Contributor

Choose a reason for hiding this comment

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

opened a followup PR that resolves this... check it #2916

// each item is rendered once
assert.equal(testProps.itemRenderer.callCount, 15);
// only filtered items re-rendered
Expand Down Expand Up @@ -65,7 +65,7 @@ export function selectComponentSuite<P extends IListItemsProps<IFilm>, S>(
});

it("clicking item resets state when resetOnSelect=true", () => {
const wrapper = render({ ...testProps, query: "19", resetOnSelect: true });
const wrapper = render({ ...testProps, query: "19", resetOnSelect: true, resetOnQuery: false });
findItems(wrapper)
.at(3)
.simulate("click");
Expand All @@ -74,6 +74,28 @@ export function selectComponentSuite<P extends IListItemsProps<IFilm>, S>(
assert.deepEqual(ranks, [5, 1]);
assert.strictEqual(testProps.onQueryChange.lastCall.args[0], "");
});

it("querying does not reset active item when resetOnQuery=false", () => {
const wrapper = render({ ...testProps, query: "19", resetOnQuery: false });

assert.strictEqual(testProps.onActiveItemChange.lastCall, null);

// querying should not select any new active item
findInput(wrapper).simulate("change", { target: { value: "Forrest" } });

assert.strictEqual(testProps.onActiveItemChange.lastCall, null);
});

it("querying resets active item when resetOnQuery=true", () => {
const wrapper = render({ ...testProps, query: "19" });

assert.strictEqual(testProps.onActiveItemChange.lastCall, null);

// querying should select Forrest Gump
findInput(wrapper).simulate("change", { target: { value: "Forrest" } });

assert.strictEqual(testProps.onActiveItemChange.lastCall.args[0].title, "Forrest Gump");
});
});

describe("keyboard", () => {
Expand Down