From 03c69fdb579977ebde1411a116645100c9621612 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Mon, 13 Jun 2022 22:50:35 +0000 Subject: [PATCH 1/3] [select] fix: various fixes --- packages/select/src/common/classes.ts | 6 ++++-- packages/select/src/common/selectPopoverProps.ts | 4 +++- .../src/components/multi-select/multiSelect2.tsx | 8 +++----- packages/select/src/components/select/select2.tsx | 10 +++------- packages/select/src/components/suggest/suggest2.tsx | 4 ++-- 5 files changed, 15 insertions(+), 17 deletions(-) diff --git a/packages/select/src/common/classes.ts b/packages/select/src/common/classes.ts index 00c9e1ee9b..fef52435eb 100644 --- a/packages/select/src/common/classes.ts +++ b/packages/select/src/common/classes.ts @@ -23,6 +23,8 @@ export const MULTISELECT_POPOVER = `${MULTISELECT}-popover`; export const MULTISELECT_TAG_INPUT_INPUT = `${MULTISELECT}-tag-input-input`; export const OMNIBAR = `${NS}-omnibar`; export const OMNIBAR_OVERLAY = `${OMNIBAR}-overlay`; +/** @deprecated this class name is not rendered by any component in this package */ export const SELECT = `${NS}-select`; -export const SELECT_POPOVER = `${SELECT}-popover`; -export const SELECT_MATCH_TARGET_WIDTH = `${SELECT}-match-target-width`; +export const SELECT_POPOVER = `${NS}-select-popover`; +export const SELECT_MATCH_TARGET_WIDTH = `${NS}-select-match-target-width`; +export const SUGGEST_POPOVER = `${NS}-suggest-popover`; diff --git a/packages/select/src/common/selectPopoverProps.ts b/packages/select/src/common/selectPopoverProps.ts index 5bbcc2e13f..8f2136b8a8 100644 --- a/packages/select/src/common/selectPopoverProps.ts +++ b/packages/select/src/common/selectPopoverProps.ts @@ -28,7 +28,9 @@ export interface SelectPopoverProps { * Props to spread to `Popover2`. * Note that `content` cannot be changed aside from utilizing `popoverContentProps`. */ - popoverProps?: Partial>; + popoverProps?: Partial< + Omit + >; /** * Optional ref for the Popover2 component instance. diff --git a/packages/select/src/components/multi-select/multiSelect2.tsx b/packages/select/src/components/multi-select/multiSelect2.tsx index 9d7badfbe6..7ee6c5552f 100644 --- a/packages/select/src/components/multi-select/multiSelect2.tsx +++ b/packages/select/src/components/multi-select/multiSelect2.tsx @@ -35,8 +35,6 @@ import { Popover2, Popover2TargetProps, PopupKind } from "@blueprintjs/popover2" import { Classes, IListItemsProps, SelectPopoverProps } from "../../common"; import { IQueryListRendererProps, QueryList } from "../query-list/queryList"; -// N.B. selectedItems should really be a required prop, but is left optional for backwards compatibility - export interface MultiSelect2Props extends IListItemsProps, SelectPopoverProps { /** * Whether the component is non-interactive. @@ -95,7 +93,7 @@ export interface MultiSelect2Props extends IListItemsProps, SelectPopoverP popoverTargetProps?: React.HTMLAttributes; /** Controlled selected values. */ - selectedItems?: T[]; + selectedItems: T[]; /** * Props to spread to `TagInput`. @@ -233,7 +231,7 @@ export class MultiSelect2 extends AbstractPureComponent2 onClear, placeholder, popoverTargetProps = {}, - selectedItems = [], + selectedItems, tagInputProps = {}, } = this.props; const { handleKeyDown, handleKeyUp } = listProps; @@ -331,7 +329,7 @@ export class MultiSelect2 extends AbstractPureComponent2 }; private handleTagRemove = (tag: React.ReactNode, index: number) => { - const { selectedItems = [], onRemove, tagInputProps } = this.props; + const { selectedItems, onRemove, tagInputProps } = this.props; onRemove?.(selectedItems[index], index); tagInputProps?.onRemove?.(tag, index); this.refHandlers.popover.current?.reposition(); // reposition when size of input changes diff --git a/packages/select/src/components/select/select2.tsx b/packages/select/src/components/select/select2.tsx index a3b86da16d..779a4ccdbf 100644 --- a/packages/select/src/components/select/select2.tsx +++ b/packages/select/src/components/select/select2.tsx @@ -28,7 +28,6 @@ import { InputGroupProps2, IRef, Keys, - Position, refHandler, setRef, Utils, @@ -147,7 +146,6 @@ export class Select2 extends AbstractPureComponent2, Select2S private renderQueryList = (listProps: IQueryListRendererProps) => { // not using defaultProps cuz they're hard to type with generics (can't use on static members) const { - fill, filterable = true, disabled = false, inputProps = {}, @@ -156,10 +154,6 @@ export class Select2 extends AbstractPureComponent2, Select2S popoverRef, } = this.props; - if (fill) { - popoverProps.fill = true; - } - const input = ( extends AbstractPureComponent2, Select2S ); const { handleKeyDown, handleKeyUp } = listProps; + + // N.B. no need to set `fill` since that is unused with the `renderTarget` API return ( extends AbstractPureComponent2, Sugges autoFocus={false} enforceFocus={false} isOpen={isOpen} - position={Position.BOTTOM_LEFT} + position={popoverProps.placement !== undefined ? undefined : "bottom-left"} {...popoverProps} className={classNames(listProps.className, popoverProps.className)} content={ @@ -197,7 +197,7 @@ export class Suggest2 extends AbstractPureComponent2, Sugges onInteraction={this.handlePopoverInteraction} onOpened={this.handlePopoverOpened} onOpening={this.handlePopoverOpening} - popoverClassName={classNames(Classes.SELECT_POPOVER, popoverProps.popoverClassName)} + popoverClassName={classNames(Classes.SUGGEST_POPOVER, popoverProps.popoverClassName)} popupKind={PopupKind.LISTBOX} ref={popoverRef} renderTarget={this.getPopoverTargetRenderer(listProps, isOpen)} From c324d605b5873a6ee6044f00680830b60770c3c5 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Mon, 13 Jun 2022 22:58:02 +0000 Subject: [PATCH 2/3] remove unused import, fix code style --- packages/select/src/components/select/select2.tsx | 2 +- packages/select/src/components/suggest/suggest2.tsx | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/select/src/components/select/select2.tsx b/packages/select/src/components/select/select2.tsx index 779a4ccdbf..ba36c8bbbb 100644 --- a/packages/select/src/components/select/select2.tsx +++ b/packages/select/src/components/select/select2.tsx @@ -176,7 +176,7 @@ export class Select2 extends AbstractPureComponent2, Select2S enforceFocus={false} isOpen={this.state.isOpen} disabled={disabled} - position={popoverProps.placement !== undefined ? undefined : "bottom-left"} + placement={popoverProps.position || popoverProps.placement ? undefined : "bottom-start"} {...popoverProps} className={classNames(listProps.className, popoverProps.className)} content={ diff --git a/packages/select/src/components/suggest/suggest2.tsx b/packages/select/src/components/suggest/suggest2.tsx index 85d955f89a..66db620067 100644 --- a/packages/select/src/components/suggest/suggest2.tsx +++ b/packages/select/src/components/suggest/suggest2.tsx @@ -27,7 +27,6 @@ import { IRef, Keys, mergeRefs, - Position, refHandler, setRef, Utils, @@ -185,7 +184,7 @@ export class Suggest2 extends AbstractPureComponent2, Sugges autoFocus={false} enforceFocus={false} isOpen={isOpen} - position={popoverProps.placement !== undefined ? undefined : "bottom-left"} + placement={popoverProps.position || popoverProps.placement ? undefined : "bottom-start"} {...popoverProps} className={classNames(listProps.className, popoverProps.className)} content={ From be40c2f89ac62b2c311abdb838f116f31179c33d Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Mon, 13 Jun 2022 23:37:14 +0000 Subject: [PATCH 3/3] fix tests --- packages/select/test/multiSelect2Tests.tsx | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/select/test/multiSelect2Tests.tsx b/packages/select/test/multiSelect2Tests.tsx index 0dd6c50fd9..b7f091fb99 100644 --- a/packages/select/test/multiSelect2Tests.tsx +++ b/packages/select/test/multiSelect2Tests.tsx @@ -51,7 +51,14 @@ describe("", () => { }); selectComponentSuite, MultiSelect2State>(props => - mount(), + mount( + , + ), ); it("placeholder can be controlled with placeholder prop", () => { @@ -76,12 +83,6 @@ describe("", () => { assert.equal(wrapper.find(Tag).find("strong").length, 1); }); - // N.B. this is not good behavior, we shouldn't support this since the component is controlled. - // we keep it around for backcompat but expect that nobody actually uses the component this way. - it("selectedItems is optional", () => { - assert.doesNotThrow(() => multiselect({ selectedItems: undefined })); - }); - it("only triggers QueryList key up events when focus is on TagInput's ", () => { const itemSelectSpy = sinon.spy(); const wrapper = multiselect({