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

[core] refactor(InputGroup): improve interface #4441

Merged
merged 7 commits into from
Jan 19, 2021
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
16 changes: 14 additions & 2 deletions packages/core/src/common/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ export interface ILinkProps {
target?: string;
}

/** Interface for a controlled input. */
/**
* @deprecated use IControlledProps2.
*
* Interface for a controlled input.
*/
export interface IControlledProps {
/** Initial value of the input, for uncontrolled usage. */
defaultValue?: string;
Expand All @@ -94,6 +98,14 @@ export interface IControlledProps {
value?: string;
}

export interface IControlledProps2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry to revisit this -- why are you making this change to remove onChange from the interface? you're Omiting those fields anyway in IInputGroupProps2 below..?

Copy link
Contributor Author

@ejose19 ejose19 Jan 12, 2021

Choose a reason for hiding this comment

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

The point of IControlledProps (as far I understand it) is to define the correct types we support, which is only value: string on value and defaultValue props, however onChange is compatible without any other change, so we're defining unnecessary properties (and not following the original typing which is React.ChangeEvent not React.FormEventHandler [even though React.ChangeEventHandler could be used]).

So unless there's an specific reason so not use the native type, I don't see why it should be redefined.

/** Initial value of the input, for uncontrolled usage. */
defaultValue?: string;

/** Form value of the input, for controlled usage. */
value?: string;
}

export interface IElementRefProps<E extends HTMLElement> {
/** A ref handler or a ref object that receives the native HTML element rendered by this component. */
elementRef?: IRef<E>;
Expand All @@ -117,7 +129,7 @@ export interface IOptionProps extends IProps {
const INVALID_PROPS = [
"active",
"alignText",
"asyncControl", // IInputGroupProps
"asyncControl", // IInputGroupProps2
"containerRef",
"current",
"elementRef",
Expand Down
91 changes: 85 additions & 6 deletions packages/core/src/components/forms/inputGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
DISPLAYNAME_PREFIX,
HTMLInputProps,
IControlledProps,
IControlledProps2,
IIntentProps,
IProps,
MaybeElement,
Expand All @@ -32,9 +33,87 @@ import {
import { Icon, IconName } from "../icon/icon";
import { AsyncControllableInput } from "./asyncControllableInput";

// NOTE: This interface does not extend HTMLInputProps due to incompatiblity with `IControlledProps`.
// Instead, we union the props in the component definition, which does work and properly disallows `string[]` values.
export interface IInputGroupProps extends IControlledProps, IIntentProps, IProps {
/**
* @deprecated use IInputGroupProps2.
*
* NOTE: This interface does not extend HTMLInputProps due to incompatiblity with `IControlledProps`.
* Instead, we union the props in the component definition, which does work and properly disallows `string[]` values.
*/

export interface IInputGroupProps
// eslint-disable-next-line deprecation/deprecation
extends IControlledProps,
IIntentProps,
IProps {
/**
* Set this to `true` if you will be controlling the `value` of this input with asynchronous updates.
* These may occur if you do not immediately call setState in a parent component with the value from
* the `onChange` handler, or if working with certain libraries like __redux-form__.
*
* @default false
*/
asyncControl?: boolean;

/**
* Whether the input is non-interactive.
* Note that `rightElement` must be disabled separately; this prop will not affect it.
*
* @default false
*/
disabled?: boolean;

/**
* Whether the component should take up the full width of its container.
*/
fill?: boolean;

/** Ref handler or a ref object that receives HTML `<input>` element backing this component. */
inputRef?: IRef<HTMLInputElement>;

/**
* Element to render on the left side of input. This prop is mutually exclusive
* with `leftIcon`.
*/
leftElement?: JSX.Element;

/**
* Name of a Blueprint UI icon to render on the left side of the input group,
* before the user's cursor. This prop is mutually exclusive with `leftElement`.
* Usage with content is deprecated. Use `leftElement` for elements.
*/
leftIcon?: IconName | MaybeElement;

/** Whether this input should use large styles. */
large?: boolean;

/** Whether this input should use small styles. */
small?: boolean;

/** Placeholder text in the absence of any value. */
placeholder?: string;

/**
* Element to render on right side of input.
* For best results, use a minimal button, tag, or small spinner.
*/
rightElement?: JSX.Element;

/** Whether the input (and any buttons) should appear with rounded caps. */
round?: boolean;

/**
* HTML `input` type attribute.
*
* @default "text"
*/
type?: string;
}

export interface IInputGroupProps2
extends Omit<HTMLInputProps, keyof IControlledProps2>,
IControlledProps2,
IIntentProps,
IProps {
/**
* Set this to `true` if you will be controlling the `value` of this input with asynchronous updates.
* These may occur if you do not immediately call setState in a parent component with the value from
Expand Down Expand Up @@ -105,7 +184,7 @@ export interface IInputGroupState {
}

@polyfill
export class InputGroup extends AbstractPureComponent2<IInputGroupProps & HTMLInputProps, IInputGroupState> {
export class InputGroup extends AbstractPureComponent2<IInputGroupProps2, IInputGroupState> {
public static displayName = `${DISPLAYNAME_PREFIX}.InputGroup`;

public state: IInputGroupState = {};
Expand Down Expand Up @@ -162,14 +241,14 @@ export class InputGroup extends AbstractPureComponent2<IInputGroupProps & HTMLIn
this.updateInputWidth();
}

public componentDidUpdate(prevProps: IInputGroupProps & HTMLInputProps) {
public componentDidUpdate(prevProps: IInputGroupProps2) {
const { leftElement, rightElement } = this.props;
if (prevProps.leftElement !== leftElement || prevProps.rightElement !== rightElement) {
this.updateInputWidth();
}
}

protected validateProps(props: IInputGroupProps) {
protected validateProps(props: IInputGroupProps2) {
if (props.leftElement != null && props.leftIcon != null) {
console.warn(Errors.INPUT_WARN_LEFT_ELEMENT_LEFT_ICON_MUTEX);
}
Expand Down
7 changes: 3 additions & 4 deletions packages/datetime/src/dateInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ import {
AbstractPureComponent2,
DISPLAYNAME_PREFIX,
getRef,
HTMLInputProps,
IInputGroupProps,
IInputGroupProps2,
InputGroup,
Intent,
IPopoverProps,
Expand Down Expand Up @@ -89,7 +88,7 @@ export interface IDateInputProps extends IDatePickerBaseProps, IDateFormatProps,
* `disabled` and `value` will be ignored in favor of the top-level props on this component.
* `type` is fixed to "text" and `ref` is not supported; use `inputRef` instead.
*/
inputProps?: HTMLInputProps & IInputGroupProps;
inputProps?: IInputGroupProps2;

/**
* Called when the user selects a new valid date through the `DatePicker` or by typing
Expand Down Expand Up @@ -475,7 +474,7 @@ export class DateInput extends AbstractPureComponent2<IDateInputProps, IDateInpu
};

/** safe wrapper around invoking input props event handler (prop defaults to undefined) */
private safeInvokeInputProp(name: keyof HTMLInputProps, e: React.SyntheticEvent<HTMLElement>) {
private safeInvokeInputProp(name: keyof IInputGroupProps2, e: React.SyntheticEvent<HTMLElement>) {
const { inputProps = {} } = this.props;
inputProps[name]?.(e);
}
Expand Down
7 changes: 3 additions & 4 deletions packages/datetime/src/dateRangeInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ import {
Classes,
DISPLAYNAME_PREFIX,
getRef,
HTMLInputProps,
IInputGroupProps,
IInputGroupProps2,
InputGroup,
Intent,
IPopoverProps,
Expand Down Expand Up @@ -97,7 +96,7 @@ export interface IDateRangeInputProps extends IDatePickerBaseProps, IDateFormatP
* `disabled` and `value` will be ignored in favor of the top-level props on this component.
* `ref` is not supported; use `inputRef` instead.
*/
endInputProps?: HTMLInputProps & IInputGroupProps;
endInputProps?: IInputGroupProps2;

/**
* Called when the user selects a day.
Expand Down Expand Up @@ -159,7 +158,7 @@ export interface IDateRangeInputProps extends IDatePickerBaseProps, IDateFormatP
* `disabled` and `value` will be ignored in favor of the top-level props on this component.
* `ref` is not supported; use `inputRef` instead.
*/
startInputProps?: HTMLInputProps & IInputGroupProps;
startInputProps?: IInputGroupProps2;

/**
* The currently selected date range.
Expand Down
4 changes: 2 additions & 2 deletions packages/datetime/test/dateRangeInputTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
Classes as CoreClasses,
HTMLDivProps,
HTMLInputProps,
IInputGroupProps,
IInputGroupProps2,
InputGroup,
IPopoverProps,
Keys,
Expand Down Expand Up @@ -258,7 +258,7 @@ describe("<DateRangeInput>", () => {

function runTestSuite(
inputGetterFn: (root: WrappedComponentRoot) => WrappedComponentInput,
mountFn: (inputGroupProps: HTMLInputProps & IInputGroupProps) => any,
mountFn: (inputGroupProps: IInputGroupProps2) => any,
) {
it("allows custom placeholder text", () => {
const root = mountFn({ placeholder: "Hello" });
Expand Down
4 changes: 2 additions & 2 deletions packages/docs-theme/src/components/navigator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { IHeadingNode, IPageNode } from "@documentalist/client";
import { filter } from "fuzzaldrin-plus";
import * as React from "react";

import { Classes, Icon, IInputGroupProps, MenuItem } from "@blueprintjs/core";
import { Classes, Icon, IInputGroupProps2, MenuItem } from "@blueprintjs/core";
import { ItemListPredicate, ItemRenderer, Omnibar } from "@blueprintjs/select";

import { eachLayoutNode } from "../common/utils";
Expand Down Expand Up @@ -47,7 +47,7 @@ export interface INavigationSection {
}

const NavOmnibar = Omnibar.ofType<INavigationSection>();
const INPUT_PROPS: IInputGroupProps = { placeholder: "Fuzzy search headings..." };
const INPUT_PROPS: IInputGroupProps2 = { placeholder: "Fuzzy search headings..." };

export class Navigator extends React.PureComponent<INavigatorProps> {
private sections: INavigationSection[];
Expand Down
11 changes: 2 additions & 9 deletions packages/select/src/components/omnibar/omnibar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,7 @@
import classNames from "classnames";
import * as React from "react";

import {
DISPLAYNAME_PREFIX,
HTMLInputProps,
IInputGroupProps,
InputGroup,
IOverlayProps,
Overlay,
} from "@blueprintjs/core";
import { DISPLAYNAME_PREFIX, IInputGroupProps2, InputGroup, IOverlayProps, Overlay } from "@blueprintjs/core";

import { Classes, IListItemsProps } from "../../common";
import { IQueryListRendererProps, QueryList } from "../query-list/queryList";
Expand All @@ -35,7 +28,7 @@ export interface IOmnibarProps<T> extends IListItemsProps<T> {
* `onQueryChange` instead of `inputProps.value` and `inputProps.onChange`
* to control this input.
*/
inputProps?: IInputGroupProps & HTMLInputProps;
inputProps?: IInputGroupProps2;

/**
* Toggles the visibility of the omnibar.
Expand Down
5 changes: 2 additions & 3 deletions packages/select/src/components/select/select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ import {
Button,
DISPLAYNAME_PREFIX,
getRef,
HTMLInputProps,
IInputGroupProps,
IInputGroupProps2,
InputGroup,
IPopoverProps,
IRef,
Expand Down Expand Up @@ -60,7 +59,7 @@ export interface ISelectProps<T> extends IListItemsProps<T> {
* `onQueryChange` instead of `inputProps.value` and `inputProps.onChange`
* to control this input.
*/
inputProps?: IInputGroupProps & HTMLInputProps;
inputProps?: IInputGroupProps2;

/** Props to spread to `Popover`. Note that `content` cannot be changed. */
// eslint-disable-next-line @typescript-eslint/ban-types
Expand Down
5 changes: 2 additions & 3 deletions packages/select/src/components/select/suggest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ import {
AbstractPureComponent2,
DISPLAYNAME_PREFIX,
getRef,
HTMLInputProps,
IInputGroupProps,
IInputGroupProps2,
InputGroup,
IPopoverProps,
IRef,
Expand Down Expand Up @@ -59,7 +58,7 @@ export interface ISuggestProps<T> extends IListItemsProps<T> {
* `query` and `onQueryChange` instead of `inputProps.value` and
* `inputProps.onChange`.
*/
inputProps?: IInputGroupProps & HTMLInputProps;
inputProps?: IInputGroupProps2;

/** Custom renderer to transform an item into a string for the input value. */
inputValueRenderer: (item: T) => string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ import {
Button,
Classes as CoreClasses,
DISPLAYNAME_PREFIX,
HTMLInputProps,
IButtonProps,
IInputGroupProps,
IInputGroupProps2,
IPopoverProps,
IProps,
MenuItem,
Expand Down Expand Up @@ -102,7 +101,7 @@ export interface ITimezonePickerProps extends IProps {
* If you want to control the filter input, you can pass `value` and `onChange` here
* to override `Select`'s own behavior.
*/
inputProps?: IInputGroupProps & HTMLInputProps;
inputProps?: IInputGroupProps2;

/** Props to spread to `Popover`. Note that `content` cannot be changed. */
popoverProps?: Partial<IPopoverProps>;
Expand Down Expand Up @@ -146,7 +145,7 @@ export class TimezonePicker extends AbstractPureComponent2<ITimezonePickerProps,
const { children, className, disabled, inputProps, popoverProps } = this.props;
const { query } = this.state;

const finalInputProps: IInputGroupProps & HTMLInputProps = {
const finalInputProps: IInputGroupProps2 = {
placeholder: "Search for timezones...",
...inputProps,
};
Expand Down
6 changes: 3 additions & 3 deletions packages/timezone/test/timezonePickerTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import * as sinon from "sinon";
import {
Button,
IButtonProps,
IInputGroupProps,
IInputGroupProps2,
InputGroup,
IPopoverProps,
MenuItem,
Expand Down Expand Up @@ -198,15 +198,15 @@ describe("<TimezonePicker>", () => {
});

it("input can be controlled with input props", () => {
const inputProps: IInputGroupProps = {
const inputProps: IInputGroupProps2 = {
disabled: true,
leftIcon: "airplane",
placeholder: "test placeholder",
};
const timezonePicker = shallow(<TimezonePicker {...DEFAULT_PROPS} inputProps={inputProps} />);
const inputGroup = findInputGroup(timezonePicker);
for (const key of Object.keys(inputProps)) {
assert.deepEqual(inputGroup.prop(key), inputProps[key as keyof IInputGroupProps]);
assert.deepEqual(inputGroup.prop(key), inputProps[key as keyof IInputGroupProps2]);
}
});

Expand Down