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.

Sign up for GitHub

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

SearchField: Add onKeyUp prop #2326

Merged
merged 1 commit into from
Sep 19, 2024
Merged

SearchField: Add onKeyUp prop #2326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions .changeset/curly-bats-cover.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-search-field": minor
---

Add onKeyUp prop to the `SearchField` component
30 changes: 8 additions & 22 deletions __docs__/wonder-blocks-search-field/search-field.argtypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
export default {
clearAriaLabel: {
description: `ARIA label for the clear button. Defaults to "Clear search".`,
Copy link
Member

Choose a reason for hiding this comment

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

praise: Thanks for cleaning this up!

I will also start removing descriptions from story argtypes as I go along so we can rely on the prop docs to avoid duplication 😄

type: {name: "string", required: false},
table: {
type: {
Expand All @@ -13,8 +12,6 @@ export default {
},
},
id: {
description: `The unique identifier for the input. If one is not
provided, a unique id will be generated.`,
type: {name: "string", required: false},
table: {
type: {
Expand All @@ -26,7 +23,6 @@ export default {
},
},
value: {
description: "The text input value.",
type: {name: "string", required: true},
table: {
type: {
Expand All @@ -36,8 +32,6 @@ export default {
control: {type: "text"},
},
name: {
description: `The name for the input control. This is submitted along
with the form data.`,
type: {name: "string", required: false},
table: {
type: {
Expand All @@ -49,9 +43,6 @@ export default {
},
},
placeholder: {
description: `Provide hints or examples of what to enter.
This shows up as a grayed out text in the field before
a value is entered.`,
type: {name: "string", required: false},
table: {
type: {
Expand All @@ -63,7 +54,6 @@ export default {
},
},
autoFocus: {
description: "Whether this field should autofocus on page load.",
type: {name: "boolean", required: false},
table: {
type: {
Expand All @@ -78,8 +68,6 @@ export default {
},
},
disabled: {
description: `Makes a read-only input field that cannot be focused.
Defaults to false.`,
type: {name: "boolean", required: false},
table: {
type: {
Expand All @@ -94,8 +82,6 @@ export default {
},
},
light: {
description:
"Change the default focus ring color to fit a dark background.",
type: {name: "boolean", required: false},
table: {
type: {
Expand All @@ -110,7 +96,6 @@ export default {
},
},
style: {
description: "Custom styles for the main wrapper.",
table: {
type: {
summary: "Style",
Expand All @@ -124,7 +109,6 @@ export default {
},
},
testId: {
description: "Test ID used for e2e testing.",
type: {name: "string", required: false},
table: {
type: {
Expand All @@ -136,7 +120,6 @@ export default {
},
},
onChange: {
description: "Called when the value has changed.",
table: {
type: {
summary: "function",
Expand All @@ -145,8 +128,6 @@ export default {
},
},
onClick: {
description: `Handler that is triggered when this component is clicked.
For example, use this to adjust focus in parent component.`,
table: {
type: {
summary: "function",
Expand All @@ -155,7 +136,14 @@ export default {
},
},
onKeyDown: {
description: "Called when a key is pressed.",
table: {
type: {
summary: "function",
},
category: "Events",
},
},
onKeyUp: {
table: {
type: {
summary: "function",
Expand All @@ -164,7 +152,6 @@ export default {
},
},
onFocus: {
description: "Called when the element has been focused.",
table: {
type: {
summary: "function",
Expand All @@ -173,7 +160,6 @@ export default {
},
},
onBlur: {
description: "Called when the element has been blurred.",
table: {
type: {
summary: "function",
Expand Down
23 changes: 23 additions & 0 deletions packages/wonder-blocks-search-field/src/components/__tests__/search-field.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,4 +418,27 @@ describe("SearchField", () => {
// Assert
expect(searchField).not.toHaveFocus();
});

it("onKeyDown is called after keyboard key press", async () => {
// Arrange
const handleOnKeyDown = jest.fn(
(event: React.KeyboardEvent<HTMLInputElement>) => {
return event.key;
},
);

render(
<SearchField
value="something"
onChange={() => {}}
onKeyDown={handleOnKeyDown}
/>,
);

// Act
await userEvent.type(await screen.findByRole("textbox"), "{enter}");

// Assert
expect(handleOnKeyDown).toHaveReturnedWith("Enter");
});
});
4 changes: 4 additions & 0 deletions packages/wonder-blocks-search-field/src/components/search-field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ type Props = AriaProps & {
* Called when a key is pressed.
*/
onKeyDown?: (event: React.KeyboardEvent<HTMLInputElement>) => unknown;
/**
* Called when a key is released.
*/
onKeyUp?: (event: React.KeyboardEvent<HTMLInputElement>) => unknown;
Copy link
Member

Choose a reason for hiding this comment

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

question: I noticed we often deconstruct the specific props before applying it to the underlying element. Do we do that so that the remaining otherProps will usually be the aria props? Are there any drawbacks to letting other props be part of the otherProps?

Here's an example where we do that with the other event handlers:

wonder-blocks/packages/wonder-blocks-search-field/src/components/search-field.tsx

Lines 126 to 130 in dd2759e

onClick,
onChange,
onFocus,
onBlur,
...otherProps

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct, we use that for aria props, and also for letting other valid attributes be injected in the inner node. I don't think there are drawbacks with that logic.... the only thing is that TS would complain, but this can be fixed by adding the missing props, like the one included here :).

/**
* Called when the element has been focused.
*/
Expand Down