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] popoverTargetProps and inputProps fixes & docs #5730

Merged
merged 4 commits into from
Nov 7, 2022
Merged
Show file tree
Hide file tree
Changes from 3 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
12 changes: 10 additions & 2 deletions packages/select/src/common/selectPopoverProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,17 @@ import type { Popover2, Popover2Props } from "@blueprintjs/popover2";
* and need to provide some degree of customization for that popover.
*/
export interface SelectPopoverProps {
/** Props to spread to `Popover2` content wrapper eleemnt. */
/**
* HTML attributes to spread to the popover content container element.
*/
popoverContentProps?: React.HTMLAttributes<HTMLDivElement>;

/**
* Props to spread to `Popover2`.
*
* Note that `content` cannot be changed, but you may apply some props to the content wrapper element
* with `popoverContentProps`.
* with `popoverContentProps`. Likewise, `targetProps` is no longer supported as it was in Blueprint v4, but you
* may use `popoverTargetProps` instead.
*/
popoverProps?: Partial<Omit<Popover2Props, "content" | "defaultIsOpen" | "disabled" | "fill" | "renderTarget">>;

Expand All @@ -37,4 +40,9 @@ export interface SelectPopoverProps {
* This is sometimes useful to reposition the popover.
*/
popoverRef?: React.RefObject<Popover2<React.HTMLProps<unknown>>>;

/**
* HTML attributes to add to the popover target element.
*/
popoverTargetProps?: React.HTMLAttributes<HTMLElement>;
}
22 changes: 11 additions & 11 deletions packages/select/src/components/multi-select/multiSelect2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export interface MultiSelect2Props<T> extends ListItemsProps<T>, SelectPopoverPr

/**
* Whether the component should take up the full width of its container.
* This overrides `popoverProps.fill` and `tagInputProps.fill`.
*/
fill?: boolean;

Expand Down Expand Up @@ -92,25 +91,22 @@ export interface MultiSelect2Props<T> extends ListItemsProps<T>, SelectPopoverPr
*/
placeholder?: string;

/**
* Props to add to the `div` that wraps the TagInput
*/
popoverTargetProps?: React.HTMLAttributes<HTMLDivElement>;

/** Controlled selected values. */
selectedItems: T[];

/**
* Props to spread to `TagInput`.
* If you wish to control the value of the input, use `query` and `onQueryChange` instead.
* Props to pass to the [TagInput component](##core/components/tag-input).
*
* Some properties are unavailable:
* - `tagInputProps.value`: use `query` instead
* - `tagInputProps.onChange`: use `onQueryChange` instead
*
* Notes for `tagInputProps.rightElement`:
* - you are responsible for disabling any elements you may render here when the overall
* `MultiSelect2` is disabled.
* - you are responsible for disabling any elements you may render here when the overall `MultiSelect2` is disabled
* - if the `onClear` prop is defined, this element will override/replace the default rightElement,
* which is a "clear" button that removes all items from the current selection.
*/
tagInputProps?: Partial<TagInputProps>;
tagInputProps?: Partial<Omit<TagInputProps, "value" | "onChange">>;

/** Custom renderer to transform an item into tag content. */
tagRenderer: (item: T) => React.ReactNode;
Expand Down Expand Up @@ -385,6 +381,8 @@ export class MultiSelect2<T> extends AbstractPureComponent2<MultiSelect2Props<T>
if (this.state.isOpen && !isTargetingTagRemoveButton) {
handleQueryListKeyDown?.(e);
}

this.props.popoverTargetProps?.onKeyDown?.(e);
};
};

Expand All @@ -397,6 +395,8 @@ export class MultiSelect2<T> extends AbstractPureComponent2<MultiSelect2Props<T>
if (this.state.isOpen && isTargetingInput) {
handleQueryListKeyUp?.(e);
}

this.props.popoverTargetProps?.onKeyDown?.(e);
};
};

Expand Down
45 changes: 31 additions & 14 deletions packages/select/src/components/select/select2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ export interface Select2Props<T> extends ListItemsProps<T>, SelectPopoverProps {

/**
* Whether the component should take up the full width of its container.
* This overrides `popoverProps.fill`. You also have to ensure that the child
* component has `fill` set to `true` or is styled appropriately.
* You also have to ensure that the child component has `fill` set to `true` or is styled appropriately.
*/
fill?: boolean;

Expand All @@ -68,22 +67,19 @@ export interface Select2Props<T> extends ListItemsProps<T>, SelectPopoverProps {
filterable?: boolean;

/**
* Props to spread to the query `InputGroup`. Use `query` and
* `onQueryChange` instead of `inputProps.value` and `inputProps.onChange`
* to control this input.
* Props to pass to the query [InputGroup component](#core/components/text-inputs.input-group).
*
* Some properties are unavailable:
* - `inputProps.value`: use `query` instead
* - `inputProps.onChange`: use `onQueryChange` instead
*/
inputProps?: InputGroupProps2;
inputProps?: Partial<Omit<InputGroupProps2, "value" | "onChange">>;

/**
* Props to spread to the `Menu` listbox containing the selectable options.
* HTML attributes to add to the `Menu` listbox containing the selectable options.
*/
menuProps?: React.HTMLAttributes<HTMLUListElement>;

/**
* Props to add to the popover target wrapper element.
*/
popoverTargetProps?: React.HTMLAttributes<HTMLDivElement>;

/**
* Whether the active item should be reset to the first matching item _when
* the popover closes_. The query will also be reset to the empty string.
Expand Down Expand Up @@ -230,8 +226,11 @@ export class Select2<T> extends AbstractPureComponent2<Select2Props<T>, Select2S
// Normally, Popover2 would also need to attach its own `onKeyDown` handler via `targetProps`,
// but in our case we fully manage that interaction and listen for key events to open/close
// the popover, so we elide it from the DOM.
onKeyDown: isOpen ? handleKeyDown : this.handleTargetKeyDown,
onKeyUp: isOpen ? handleKeyUp : undefined,
onKeyDown: this.withPopoverTargetPropsHandler(
"keydown",
isOpen ? handleKeyDown : this.handleTargetKeyDown,
),
onKeyUp: this.withPopoverTargetPropsHandler("keyup", isOpen ? handleKeyUp : undefined),
ref,
role: "combobox",
},
Expand All @@ -251,6 +250,24 @@ export class Select2<T> extends AbstractPureComponent2<Select2Props<T>, Select2S
) : undefined;
}

private withPopoverTargetPropsHandler = (
eventType: "keydown" | "keyup",
handler: React.KeyboardEventHandler<HTMLElement> | undefined,
): React.KeyboardEventHandler<HTMLElement> => {
switch (eventType) {
case "keydown":
return event => {
handler?.(event);
this.props.popoverTargetProps?.onKeyDown?.(event);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a bugfix -- popoverTargetProps.onKey{Down|Up} handlers were never getting called before.

};
case "keyup":
return event => {
handler?.(event);
this.props.popoverTargetProps?.onKeyUp?.(event);
};
}
};

/**
* Target wrapper element "keydown" handler while the popover is closed.
*/
Expand Down
15 changes: 9 additions & 6 deletions packages/select/src/components/suggest/suggest2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { Popover2, Popover2TargetProps, PopupKind } from "@blueprintjs/popover2"
import { Classes, ListItemsProps, SelectPopoverProps } from "../../common";
import { QueryList, QueryListRendererProps } from "../query-list/queryList";

export interface Suggest2Props<T> extends ListItemsProps<T>, SelectPopoverProps {
export interface Suggest2Props<T> extends ListItemsProps<T>, Omit<SelectPopoverProps, "popoverTargetProps"> {
/**
* Whether the popover should close after selecting an item.
*
Expand All @@ -48,7 +48,6 @@ export interface Suggest2Props<T> extends ListItemsProps<T>, SelectPopoverProps

/**
* Whether the component should take up the full width of its container.
* This overrides `popoverProps.fill` and `inputProps.fill`.
*/
fill?: boolean;

Expand All @@ -59,10 +58,13 @@ export interface Suggest2Props<T> extends ListItemsProps<T>, SelectPopoverProps
* - `inputProps.value`: use `query` instead
* - `inputProps.onChange`: use `onQueryChange` instead
* - `inputProps.disabled`: use `disabled` instead
* - `inputProps.fill`: use `fill` instead
*
* Note that `inputProps.tagName` will override `popoverProps.targetTagName`.
* Other notes:
* - `inputProps.tagName` will override `popoverProps.targetTagName`
* - `inputProps.className` will work as expected, but this is redundant with the simpler `className` prop
*/
inputProps?: Partial<Omit<InputGroupProps2, "disabled" | "value" | "onChange">>;
inputProps?: Partial<Omit<InputGroupProps2, "disabled" | "fill" | "value" | "onChange">>;

/** Custom renderer to transform an item into a string for the input value. */
inputValueRenderer: (item: T) => string;
Expand All @@ -81,7 +83,7 @@ export interface Suggest2Props<T> extends ListItemsProps<T>, SelectPopoverProps
selectedItem?: T | null;

/**
* Props to spread to the `Menu` listbox containing the selectable options.
* HTML attributes to add to the `Menu` listbox containing the selectable options.
*/
menuProps?: React.HTMLAttributes<HTMLUListElement>;

Expand Down Expand Up @@ -225,7 +227,7 @@ export class Suggest2<T> extends AbstractPureComponent2<Suggest2Props<T>, Sugges
({
// pull out `isOpen` so that it's not forwarded to the DOM
isOpen: _isOpen,
// pull out `defaultValue` due to type incompatibility with InputGroup.
// pull out `defaultValue` due to type incompatibility with InputGroup
defaultValue,
ref,
...targetProps
Expand All @@ -252,6 +254,7 @@ export class Suggest2<T> extends AbstractPureComponent2<Suggest2Props<T>, Sugges
{...inputProps}
aria-autocomplete="list"
aria-expanded={isOpen}
className={classNames(targetProps.className, inputProps.className)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is also a bugfix -- inputProps.className was clobbering targetProps.className before, which would totally break things.

fill={fill}
inputRef={mergeRefs(this.handleInputRef, ref)}
onChange={listProps.handleQueryChange}
Expand Down