Skip to content

Commit

Permalink
feat: fix PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
YossiSaadi committed Apr 17, 2024
1 parent 04186f3 commit 1f7d460
Show file tree
Hide file tree
Showing 11 changed files with 61 additions and 91 deletions.
30 changes: 18 additions & 12 deletions packages/core/src/components/BaseInput/BaseInput.tsx
Original file line number Diff line number Diff line change
@@ -1,43 +1,49 @@
import React, { forwardRef } from "react";
import cx from "classnames";
import styles from "./BaseInput.module.scss";
import { BaseInputComponent } from "./BaseInput.types";
import { BaseInputProps } from "./BaseInput.types";
import { getStyle } from "../../helpers/typesciptCssModulesHelper";

const BaseInput: BaseInputComponent = forwardRef(
const BaseInput = forwardRef(
(
{
size = "medium",
leftRender,
rightRender,
renderLeft,
renderRight,
success,
error,
wrapperRole,
inputRole,
className,
wrapperClassName,
inputClassName,
...props
},
ref
}: BaseInputProps,
ref: React.ForwardedRef<HTMLInputElement>
) => {
const wrapperClassNames = cx(
styles.wrapper,
{
[styles.rightThinnerPadding]: !rightRender,
[styles.rightThinnerPadding]: !renderRight,
[styles.error]: error,
[styles.success]: success,
[styles.readOnly]: props.readOnly,
[styles.disabled]: props.disabled
},
getStyle(styles, size),
wrapperClassName
className
);

return (
<div className={wrapperClassNames} role={wrapperRole}>
{leftRender}
<input {...props} ref={ref} className={cx(styles.input, className)} aria-invalid={error} role={inputRole} />
{rightRender}
{renderLeft}
<input
{...props}
ref={ref}
className={cx(styles.input, inputClassName)}
aria-invalid={error}
role={inputRole}
/>
{renderRight}
</div>
);
}
Expand Down
9 changes: 3 additions & 6 deletions packages/core/src/components/BaseInput/BaseInput.types.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
import { AriaRole, InputHTMLAttributes, ReactNode } from "react";
import { VibeComponentProps } from "../../types";
import { BASE_SIZES } from "../../constants";
import VibeComponent from "../../types/VibeComponent";

export type InputSize = (typeof BASE_SIZES)[keyof typeof BASE_SIZES];
type BaseInputNativeInputProps = Omit<InputHTMLAttributes<HTMLInputElement>, "size" | "role">;
type Renderer = ReactNode | ReactNode[];

export interface BaseInputProps extends BaseInputNativeInputProps, VibeComponentProps {
size?: InputSize;
leftRender?: Renderer;
rightRender?: Renderer;
renderLeft?: Renderer;
renderRight?: Renderer;
success?: boolean;
error?: boolean;
wrapperRole?: AriaRole;
inputRole?: AriaRole;
wrapperClassName?: string;
inputClassName?: string;
}

export type BaseInputComponent = VibeComponent<BaseInputProps, HTMLInputElement>;
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ describe("BaseInput", () => {
});

it("should show left and right elements when provided", () => {
const leftRender = <div>Left</div>;
const rightRender = <div>Right</div>;
const { getByText } = renderBaseInput({ leftRender, rightRender });
const renderLeft = <div>Left</div>;
const renderRight = <div>Right</div>;
const { getByText } = renderBaseInput({ renderLeft, renderRight });

expect(getByText("Left")).toBeInTheDocument();
expect(getByText("Right")).toBeInTheDocument();
Expand All @@ -42,7 +42,7 @@ describe("BaseInput", () => {
});

it("should apply the className for input and wrapperClassName for wrapper", () => {
const { getByLabelText } = renderBaseInput({ className: "inputClass", wrapperClassName: "customWrapper" });
const { getByLabelText } = renderBaseInput({ className: "customWrapper", inputClassName: "inputClass" });
expect(getByLabelText("base-input")).toHaveClass("inputClass");
expect(getByLabelText("base-input").parentNode).toHaveClass("customWrapper");
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const ComponentRuleSimpleActions = () => (

export const ComponentRuleWithSearch = () => (
<DialogContentContainer>
<Search size={Search.sizes.SMALL} wrapperClassName={styles["component-rule-search"]} />
<Search size={Search.sizes.SMALL} className={styles["component-rule-search"]} />
<Menu>
<MenuItem title="Item 1" icon={Calendar} />
<MenuItem title="Item 2" icon={Wand} />
Expand Down
38 changes: 18 additions & 20 deletions packages/core/src/components/Search/Search.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@ import BaseInput from "../BaseInput/BaseInput";
import useDebounceEvent from "../../hooks/useDebounceEvent";
import IconButton from "../IconButton/IconButton";
import Icon from "../Icon/Icon";
import { SearchComponent } from "./Search.types";
import { SearchProps } from "./Search.types";
import Loader from "../Loader/Loader";

const Search: SearchComponent = forwardRef(
const Search = forwardRef(
(
{
searchIconName = SearchIcon,
clearIconName = CloseSmallIcon,
clearIconLabel = "Clear",
additionalActionRender: AdditionalAction,
renderAction: RenderAction,
value,
placeholder,
size = "medium",
Expand All @@ -29,27 +29,26 @@ const Search: SearchComponent = forwardRef(
inputAriaLabel,
debounceRate = 400,
searchResultsContainerId,
activeDescendant,
currentAriaResultId,
onChange,
onFocus,
onBlur,
wrapperClassName,
className,
id,
"data-testid": dataTestId
},
ref
}: SearchProps,
ref: React.ForwardedRef<HTMLInputElement>
) => {
const inputRef = useRef<HTMLInputElement>(null);
const mergedRef = useMergeRef<HTMLInputElement>(ref, inputRef);
const inputRef = useRef(null);
const mergedRef = useMergeRef(ref, inputRef);

const { inputValue, onEventChanged, clearValue } = useDebounceEvent({
delay: debounceRate,
onChange,
initialStateValue: value
});

const onClearIconClick = useCallback(() => {
const onClearButtonClick = useCallback(() => {
if (disabled) {
return;
}
Expand All @@ -65,7 +64,6 @@ const Search: SearchComponent = forwardRef(
clickable={false}
iconType={Icon.type.ICON_FONT}
iconSize={size === "small" ? "16px" : "20px"}
data-testid={getTestId(ComponentDefaultTestId.SEARCH_ICON, id)}
/>
);

Expand All @@ -74,23 +72,23 @@ const Search: SearchComponent = forwardRef(
className={cx(styles.icon, { [styles.empty]: !inputValue })}
icon={clearIconName}
ariaLabel={clearIconLabel}
onClick={onClearIconClick}
onClick={onClearButtonClick}
size={size === "small" ? IconButton.sizes.XS : IconButton.sizes.SMALL}
data-testid={getTestId(ComponentDefaultTestId.CLEAN_SEARCH_BUTTON, id)}
/>
);

const RightRender = (
const RenderRight = (
<>
{loading && (
<Loader
size={size === "small" ? Loader.sizes.XS : 20}
color={Loader.colors.SECONDARY}
wrapperClassName={cx({ [styles.loader]: !inputValue && !AdditionalAction })}
wrapperClassName={cx({ [styles.loader]: !inputValue && !RenderAction })}
/>
)}
{inputValue && !disabled && ClearIcon}
{AdditionalAction}
{RenderAction}
</>
);

Expand All @@ -100,11 +98,11 @@ const Search: SearchComponent = forwardRef(
id={id}
type={"search"}
data-testid={dataTestId || getTestId(ComponentDefaultTestId.SEARCH, id)}
className={cx(styles.search, className)}
wrapperClassName={cx(styles.searchWrapper, wrapperClassName)}
className={cx(styles.searchWrapper, className)}
inputClassName={styles.search}
value={inputValue}
leftRender={SearchIcon}
rightRender={RightRender}
renderLeft={SearchIcon}
renderRight={RenderRight}
autoFocus={autoFocus}
placeholder={placeholder}
disabled={disabled}
Expand All @@ -118,7 +116,7 @@ const Search: SearchComponent = forwardRef(
aria-label={inputAriaLabel}
aria-busy={loading}
aria-owns={searchResultsContainerId}
aria-activedescendant={activeDescendant}
aria-activedescendant={currentAriaResultId}
/>
);
}
Expand Down
22 changes: 7 additions & 15 deletions packages/core/src/components/Search/Search.types.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import { SubIcon, VibeComponent, VibeComponentProps } from "../../types";
import { ReactElement, FocusEvent, AriaAttributes } from "react";
import React from "react";
import { SubIcon, VibeComponentProps } from "../../types";
import { InputSize } from "../BaseInput/BaseInput.types";
import IconButton from "../IconButton/IconButton";
import MenuButton from "../MenuButton/MenuButton";

type AdditionalActionRender = ReactElement<typeof IconButton | typeof MenuButton>;

export interface SearchProps extends VibeComponentProps {
/**
* Name of the icon used for the search button.
Expand All @@ -22,7 +20,7 @@ export interface SearchProps extends VibeComponentProps {
/**
* Render additional action within the right section of search component.
*/
additionalActionRender?: AdditionalActionRender;
renderAction?: React.ReactElement<typeof IconButton | typeof MenuButton>;
/**
* The value of the search input.
*/
Expand Down Expand Up @@ -54,7 +52,7 @@ export interface SearchProps extends VibeComponentProps {
/**
* Aria-label for the search input, important for accessibility.
*/
inputAriaLabel?: AriaAttributes["aria-label"];
inputAriaLabel?: React.AriaAttributes["aria-label"];
/**
* Rate at which search input changes are debounced.
*/
Expand All @@ -66,23 +64,17 @@ export interface SearchProps extends VibeComponentProps {
/**
* ARIA property that identifies the currently active item within the search results.
*/
activeDescendant?: AriaAttributes["aria-activedescendant"];
currentAriaResultId?: React.AriaAttributes["aria-activedescendant"];
/**
* Callback function that is called whenever the value of the search input changes.
*/
onChange?: (value: string) => void;
/**
* Callback function that is called when the search input loses focus.
*/
onBlur?: (event: FocusEvent) => void;
onBlur?: (event: React.FocusEvent) => void;
/**
* Callback function that is called when the search input gains focus.
*/
onFocus?: (event: FocusEvent) => void;
/**
* Additional className to apply to the search component's wrapper element.
*/
wrapperClassName?: string;
onFocus?: (event: React.FocusEvent) => void;
}

export type SearchComponent = VibeComponent<SearchProps, HTMLInputElement>;
14 changes: 4 additions & 10 deletions packages/core/src/components/Search/__stories__/Search.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,13 @@ The search field can contain one more search-related action (like filtering). Th

<Canvas of={SearchStories.WithAdditionalAction} />

### With clear button

Once the user starts typing, include a clear button that lets the user delete their current search quickly.

<Canvas of={SearchStories.WithClearButton} />

## Do's and Don'ts

<ComponentRules
rules={[
{
positive: {
component: <Search value="User typing" wrapperClassName={styles.fixedWidth} />,
component: <Search value="User typing" className={styles.fixedWidth} />,
description: "Keep the search icon on the left side of the input, and the clear button on the right side."
},
negative: {
Expand All @@ -96,20 +90,20 @@ Once the user starts typing, include a clear button that lets the user delete th
value="User typing"
searchIconName={CloseSmall}
clearIconName={SearchIcon}
wrapperClassName={styles.fixedWidth}
className={styles.fixedWidth}
/>
),
description: "Don't place the search icon on the right side of the input, or the clear button on the left side."
}
},
{
positive: {
component: <Search value="User typin" wrapperClassName={styles.fixedWidth} />,
component: <Search value="User typing" className={styles.fixedWidth} />,
description:
"Show clear button when the user starts typing, and keep it visible even after a search is performed."
},
negative: {
component: <Search value="User typing" clearIconName={null} wrapperClassName={styles.fixedWidth} />,
component: <Search value="User typing" clearIconName={null} className={styles.fixedWidth} />,
description: "Don't remove the clear button after the user has performed the search."
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,6 @@ export const WithAdditionalAction: Story = {
decorators: [withFixedWidth]
};

export const WithClearButton: Story = {
render: () => <Search placeholder="Placeholder text here" value="Typing" />,

decorators: [withFixedWidth]
};

const options = [
{
id: "1",
Expand Down
15 changes: 5 additions & 10 deletions packages/core/src/components/Search/__tests__/Search.jest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe("Search", () => {

it("should display the search icon by default", () => {
const { getByTestId } = renderSearch();
expect(getByTestId("search-icon")).toBeInTheDocument();
expect(getByTestId("icon")).toBeInTheDocument();
});

it("should not display the clear icon when the input is empty", () => {
Expand All @@ -26,8 +26,8 @@ describe("Search", () => {
});

it("should display both the search icon and clear icon when input has value", async () => {
const { getByTestId } = renderSearch({ value: "Test" });
expect(getByTestId("search-icon")).toBeInTheDocument();
const { getByTestId, getAllByTestId } = renderSearch({ value: "Test" });
expect(getAllByTestId("icon")).toHaveLength(2);
expect(getByTestId("clean-search-button")).toBeInTheDocument();
});

Expand All @@ -48,11 +48,6 @@ describe("Search", () => {
expect(getByTestId("loader")).toBeInTheDocument();
});

it("should apply the wrapperClassName to the wrapper element", () => {
const { container } = renderSearch({ wrapperClassName: "customWrapper" });
expect(container.firstChild).toHaveClass("customWrapper");
});

it("should trigger onChange with the correct value when typing without debounce", () => {
const onChange = jest.fn();
const { getByRole } = renderSearch({ onChange, debounceRate: 0 });
Expand Down Expand Up @@ -93,7 +88,7 @@ describe("Search", () => {

it("should display additional action render if provided", () => {
const AdditionalActionButton = <button type="button">Extra Action</button>;
const { getByText } = renderSearch({ additionalActionRender: AdditionalActionButton });
const { getByText } = renderSearch({ renderAction: AdditionalActionButton });
expect(getByText("Extra Action")).toBeInTheDocument();
});

Expand All @@ -114,7 +109,7 @@ describe("Search", () => {
});

it("should set aria-activedescendant when activeDescendant is provided", () => {
const { getByRole } = renderSearch({ activeDescendant: "option-1" });
const { getByRole } = renderSearch({ currentAriaResultId: "option-1" });
expect(getByRole("searchbox")).toHaveAttribute("aria-activedescendant", "option-1");
});

Expand Down
Loading

0 comments on commit 1f7d460

Please sign in to comment.