Skip to content

Commit

Permalink
[select] consistency refactors (#3305)
Browse files Browse the repository at this point in the history 
* code consistency across `select` components:

- public state = ... (not inconstructor)
- non-optional refs
- safeInvokeMember to reduce destructuring

* fix tests, explicit null check
  • Loading branch information
@giladgray
giladgray authored Jan 23, 2019
1 parent 10e4eb3 commit 670aabb
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 76 deletions.
5 changes: 2 additions & 3 deletions packages/select/src/components/omnibar/omnibar.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export class Omnibar<T> extends React.PureComponent<IOmnibarProps<T>> {
}

private TypedQueryList = QueryList.ofType<T>();
private queryList?: QueryList<T> | null;
private queryList: QueryList<T> | null = null;
private refHandlers = {
queryList: (ref: QueryList<T> | null) => (this.queryList = ref),
};
Expand Down Expand Up @@ -107,8 +107,7 @@ export class Omnibar<T> extends React.PureComponent<IOmnibarProps<T>> {
};

private handleOverlayClose = (event?: React.SyntheticEvent<HTMLElement>) => {
const { overlayProps = {} } = this.props;
Utils.safeInvoke(overlayProps.onClose, event);
Utils.safeInvokeMember(this.props.overlayProps, "onClose", event);
Utils.safeInvoke(this.props.onClose, event);
};
}
15 changes: 6 additions & 9 deletions packages/select/src/components/select/multiSelect.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,12 @@ export class MultiSelect<T> extends React.PureComponent<IMultiSelectProps<T>, IM
};

private TypedQueryList = QueryList.ofType<T>();
private input?: HTMLInputElement | null;
private queryList?: QueryList<T> | null;
private input: HTMLInputElement | null = null;
private queryList: QueryList<T> | null = null;
private refHandlers = {
input: (ref: HTMLInputElement | null) => {
this.input = ref;
const { tagInputProps = {} } = this.props;
Utils.safeInvoke(tagInputProps.inputRef, ref);
Utils.safeInvokeMember(this.props.tagInputProps, "inputRef", ref);
},
queryList: (ref: QueryList<T> | null) => (this.queryList = ref),
};
Expand Down Expand Up @@ -142,26 +141,24 @@ export class MultiSelect<T> extends React.PureComponent<IMultiSelectProps<T>, IM
};

private handlePopoverInteraction = (nextOpenState: boolean) =>
// deferring to rAF to get properly updated document.activeElement
requestAnimationFrame(() => {
// deferring to rAF to get properly updated activeElement
const { popoverProps = {} } = this.props;
if (this.input != null && this.input !== document.activeElement) {
// the input is no longer focused so we can close the popover
this.setState({ isOpen: false });
} else if (!this.props.openOnKeyDown) {
// open the popover when focusing the tag input
this.setState({ isOpen: true });
}
Utils.safeInvoke(popoverProps.onInteraction, nextOpenState);
Utils.safeInvokeMember(this.props.popoverProps, "onInteraction", nextOpenState);
});

private handlePopoverOpened = (node: HTMLElement) => {
const { popoverProps = {} } = this.props;
if (this.queryList != null) {
// scroll active item into view after popover transition completes and all dimensions are stable.
this.queryList.scrollActiveItemIntoView();
}
Utils.safeInvoke(popoverProps.onOpened, node);
Utils.safeInvokeMember(this.props.popoverProps, "onOpened", node);
};

private getTargetKeyDownHandler = (
Expand Down
42 changes: 16 additions & 26 deletions packages/select/src/components/select/select.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,25 +66,20 @@ export class Select<T> extends React.PureComponent<ISelectProps<T>, ISelectState
return Select as new (props: ISelectProps<T>) => Select<T>;
}

public state: ISelectState = { isOpen: false };

private TypedQueryList = QueryList.ofType<T>();
private input?: HTMLInputElement | null;
private list?: QueryList<T> | null;
private input: HTMLInputElement | null = null;
private queryList: QueryList<T> | null = null;
private previousFocusedElement: HTMLElement | undefined;
private refHandlers = {
input: (ref: HTMLInputElement | null) => {
this.input = ref;

const { inputProps = {} } = this.props;
Utils.safeInvoke(inputProps.inputRef, ref);
Utils.safeInvokeMember(this.props.inputProps, "inputRef", ref);
},
queryList: (ref: QueryList<T> | null) => (this.list = ref),
queryList: (ref: QueryList<T> | null) => (this.queryList = ref),
};

constructor(props: ISelectProps<T>, context?: any) {
super(props, context);
this.state = { isOpen: false };
}

public render() {
// omit props specific to this component, spread the rest.
const { filterable, inputProps, popoverProps, ...restProps } = this.props;
Expand All @@ -100,8 +95,8 @@ export class Select<T> extends React.PureComponent<ISelectProps<T>, ISelectState
}

public componentDidUpdate(_prevProps: ISelectProps<T>, prevState: ISelectState) {
if (this.state.isOpen && !prevState.isOpen && this.list != null) {
this.list.scrollActiveItemIntoView();
if (this.state.isOpen && !prevState.isOpen && this.queryList != null) {
this.queryList.scrollActiveItemIntoView();
}
}

Expand Down Expand Up @@ -170,27 +165,24 @@ export class Select<T> extends React.PureComponent<ISelectProps<T>, ISelectState

private handlePopoverInteraction = (isOpen: boolean) => {
this.setState({ isOpen });

const { popoverProps = {} } = this.props;
Utils.safeInvoke(popoverProps.onInteraction, isOpen);
Utils.safeInvokeMember(this.props.popoverProps, "onInteraction", isOpen);
};

private handlePopoverOpening = (node: HTMLElement) => {
const { popoverProps = {}, resetOnClose } = this.props;
// save currently focused element before popover steals focus, so we can restore it when closing.
this.previousFocusedElement = document.activeElement as HTMLElement;

if (resetOnClose) {
if (this.props.resetOnClose) {
this.resetQuery();
}

Utils.safeInvoke(popoverProps.onOpening, node);
Utils.safeInvokeMember(this.props.popoverProps, "onOpening", node);
};

private handlePopoverOpened = (node: HTMLElement) => {
// scroll active item into view after popover transition completes and all dimensions are stable.
if (this.list != null) {
this.list.scrollActiveItemIntoView();
if (this.queryList != null) {
this.queryList.scrollActiveItemIntoView();
}

requestAnimationFrame(() => {
Expand All @@ -201,8 +193,7 @@ export class Select<T> extends React.PureComponent<ISelectProps<T>, ISelectState
}
});

const { popoverProps = {} } = this.props;
Utils.safeInvoke(popoverProps.onOpened, node);
Utils.safeInvokeMember(this.props.popoverProps, "onOpened", node);
};

private handlePopoverClosing = (node: HTMLElement) => {
Expand All @@ -215,9 +206,8 @@ export class Select<T> extends React.PureComponent<ISelectProps<T>, ISelectState
}
});

const { popoverProps = {} } = this.props;
Utils.safeInvoke(popoverProps.onClosing, node);
Utils.safeInvokeMember(this.props.popoverProps, "onClosing", node);
};

private resetQuery = () => this.list && this.list.setQuery("", true);
private resetQuery = () => this.queryList && this.queryList.setQuery("", true);
}
51 changes: 16 additions & 35 deletions packages/select/src/components/select/suggest.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,26 +89,22 @@ export class Suggest<T> extends React.PureComponent<ISuggestProps<T>, ISuggestSt
return Suggest as new (props: ISuggestProps<T>) => Suggest<T>;
}

public state: ISuggestState<T> = {
isOpen: (this.props.popoverProps != null && this.props.popoverProps.isOpen) || false,
selectedItem: this.getInitialSelectedItem(),
};

private TypedQueryList = QueryList.ofType<T>();
private input: HTMLInputElement | null = null;
private queryList: QueryList<T> | null = null;
private refHandlers = {
input: (ref: HTMLInputElement | null) => {
this.input = ref;
const { inputProps = {} } = this.props;
Utils.safeInvoke(inputProps.inputRef, ref);
Utils.safeInvokeMember(this.props.inputProps, "inputRef", ref);
},
queryList: (ref: QueryList<T> | null) => (this.queryList = ref),
};

constructor(props: ISuggestProps<T>, context?: any) {
super(props, context);
this.state = {
isOpen: (props.popoverProps && props.popoverProps.isOpen) || false,
selectedItem: this.getInitialSelectedItem(),
};
}

public render() {
// omit props specific to this component, spread the rest.
const { disabled, inputProps, popoverProps, ...restProps } = this.props;
Expand Down Expand Up @@ -192,16 +188,14 @@ export class Suggest<T> extends React.PureComponent<ISuggestProps<T>, ISuggestSt
};

private handleInputFocus = (event: React.FocusEvent<HTMLInputElement>) => {
const { openOnKeyDown, inputProps = {} } = this.props;

this.selectText();

// TODO can we leverage Popover.openOnTargetFocus for this?
if (!openOnKeyDown) {
if (!this.props.openOnKeyDown) {
this.setState({ isOpen: true });
}

Utils.safeInvoke(inputProps.onFocus, event);
Utils.safeInvokeMember(this.props.inputProps, "onFocus", event);
};

private handleItemSelect = (item: T, event?: React.SyntheticEvent<HTMLElement>) => {
Expand Down Expand Up @@ -245,55 +239,43 @@ export class Suggest<T> extends React.PureComponent<ISuggestProps<T>, ISuggestSt

private handlePopoverInteraction = (nextOpenState: boolean) =>
requestAnimationFrame(() => {
const { popoverProps = {} } = this.props;

if (this.input != null && this.input !== document.activeElement) {
// the input is no longer focused so we can close the popover
this.setState({ isOpen: false });
}

Utils.safeInvoke(popoverProps.onInteraction, nextOpenState);
Utils.safeInvokeMember(this.props.popoverProps, "onInteraction", nextOpenState);
});

private handlePopoverOpening = (node: HTMLElement) => {
const { popoverProps = {}, resetOnClose } = this.props;

// reset query before opening instead of when closing to prevent flash of unfiltered items.
// this is a limitation of the interactions between QueryList state and Popover transitions.
if (resetOnClose && this.queryList) {
if (this.props.resetOnClose && this.queryList) {
this.queryList.setQuery("", true);
}

Utils.safeInvoke(popoverProps.onOpening, node);
Utils.safeInvokeMember(this.props.popoverProps, "onOpening", node);
};

private handlePopoverOpened = (node: HTMLElement) => {
const { popoverProps = {} } = this.props;

// scroll active item into view after popover transition completes and all dimensions are stable.
if (this.queryList != null) {
this.queryList.scrollActiveItemIntoView();
}

Utils.safeInvoke(popoverProps.onOpened, node);
Utils.safeInvokeMember(this.props.popoverProps, "onOpened", node);
};

private getTargetKeyDownHandler = (
handleQueryListKeyDown: React.EventHandler<React.KeyboardEvent<HTMLElement>>,
) => {
return (evt: React.KeyboardEvent<HTMLInputElement>) => {
const { which } = evt;
const { inputProps = {}, openOnKeyDown } = this.props;

if (which === Keys.ESCAPE || which === Keys.TAB) {
if (this.input != null) {
this.input.blur();
}
this.setState({
isOpen: false,
});
this.setState({ isOpen: false });
} else if (
openOnKeyDown &&
this.props.openOnKeyDown &&
which !== Keys.BACKSPACE &&
which !== Keys.ARROW_LEFT &&
which !== Keys.ARROW_RIGHT
Expand All @@ -305,17 +287,16 @@ export class Suggest<T> extends React.PureComponent<ISuggestProps<T>, ISuggestSt
Utils.safeInvoke(handleQueryListKeyDown, evt);
}

Utils.safeInvoke(inputProps.onKeyDown, evt);
Utils.safeInvokeMember(this.props.inputProps, "onKeyDown", evt);
};
};

private getTargetKeyUpHandler = (handleQueryListKeyUp: React.EventHandler<React.KeyboardEvent<HTMLElement>>) => {
return (evt: React.KeyboardEvent<HTMLInputElement>) => {
const { inputProps = {} } = this.props;
if (this.state.isOpen) {
Utils.safeInvoke(handleQueryListKeyUp, evt);
}
Utils.safeInvoke(inputProps.onKeyUp, evt);
Utils.safeInvokeMember(this.props.inputProps, "onKeyUp", evt);
};
};
}
6 changes: 3 additions & 3 deletions packages/timezone/test/timezonePickerTests.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
Popover,
Position,
} from "@blueprintjs/core";
import { IQueryListProps, ISelectProps, QueryList, Select } from "@blueprintjs/select";
import { QueryList, Select } from "@blueprintjs/select";
import {
getInitialTimezoneItems,
getLocalTimezoneItem,
Expand Down Expand Up @@ -214,13 +214,13 @@ describe("<TimezonePicker>", () => {
});

function findSelect(timezonePicker: TimezonePickerShallowWrapper) {
return timezonePicker.find<ISelectProps<ITimezoneItem>>(Select);
return timezonePicker.find(Select.ofType<ITimezoneItem>());
}

function findQueryList(timezonePicker: TimezonePickerShallowWrapper) {
return findSelect(timezonePicker)
.shallow()
.find<IQueryListProps<ITimezoneItem>>(QueryList);
.find(QueryList.ofType<ITimezoneItem>());
}

function findPopover(timezonePicker: TimezonePickerShallowWrapper) {
Expand Down

1 comment on commit 670aabb

@blueprint-bot
Copy link

Choose a reason for hiding this comment

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

[select] consistency refactors (#3305)

Previews: documentation | landing | table

Please sign in to comment.