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

[ChipDelete][joy] Add onDelete prop to ChipDelete #35412

Merged
merged 19 commits into from Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/data/joy/components/chip/chip-pt.md
Expand Up @@ -44,7 +44,8 @@ Use the `startDecorator` and/or `endDecorator` props to add supporting icons to

### Delete button

To add a delete action inside a chip, use the complementary `ChipDelete` component. Note that its design will automatically adapt to the parent `Chip`.
To add a delete action inside a chip, use the complementary `ChipDelete` component.
The `onDelete` callback is fired on `ChipDelete` when `Backspace`, `Enter` or `Delete` is pressed or when `ChipDelete` is clicked. Note that its design will automatically adapt to the parent `Chip`.
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved

```jsx
import ChipDelete from '@mui/joy/ChipDelete';
Expand Down
3 changes: 2 additions & 1 deletion docs/data/joy/components/chip/chip-zh.md
Expand Up @@ -44,7 +44,8 @@ Use the `startDecorator` and/or `endDecorator` props to add supporting icons to

### Delete button

To add a delete action inside a chip, use the complementary `ChipDelete` component. Note that its design will automatically adapt to the parent `Chip`.
To add a delete action inside a chip, use the complementary `ChipDelete` component.
The `onDelete` callback is fired on `ChipDelete` when `Backspace`, `Enter` or `Delete` is pressed or when `ChipDelete` is clicked. Note that its design will automatically adapt to the parent `Chip`.
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved

```jsx
import ChipDelete from '@mui/joy/ChipDelete';
Expand Down
2 changes: 1 addition & 1 deletion docs/data/joy/components/chip/chip.md
Expand Up @@ -49,7 +49,7 @@ Use the `startDecorator` and/or `endDecorator` props to add supporting icons to
### Delete button

To add a delete action inside a chip, use the complementary `ChipDelete` component.
Note that its design will automatically adapt to the parent `Chip`.
The `onDelete` callback is fired on `ChipDelete` when `Backspace`, `Enter` or `Delete` is pressed or when `ChipDelete` is clicked. Note that its design will automatically adapt to the parent `Chip`.

```jsx
import ChipDelete from '@mui/joy/ChipDelete';
Expand Down
31 changes: 30 additions & 1 deletion packages/mui-joy/src/ChipDelete/ChipDelete.test.js
@@ -1,6 +1,7 @@
import * as React from 'react';
import { expect } from 'chai';
import { createRenderer, describeConformance } from 'test/utils';
import { spy } from 'sinon';
import { createRenderer, describeConformance, act, fireEvent } from 'test/utils';
import { ThemeProvider } from '@mui/joy/styles';
import Chip from '@mui/joy/Chip';
import ChipDelete, { chipDeleteClasses as classes } from '@mui/joy/ChipDelete';
Expand Down Expand Up @@ -67,4 +68,32 @@ describe('<ChipDelete />', () => {
expect(getByRole('button')).to.have.class(classes.colorNeutral);
});
});
describe('Chip onDelete', () => {
it('should call onDelete function when backspace, enter or delete is pressed', () => {
const handleDelete = spy();
const { getByRole } = render(<ChipDelete onDelete={handleDelete} onClick={() => {}} />);
const chipDelete = getByRole('button');
act(() => {
chipDelete.focus();
});
fireEvent.keyDown(chipDelete, { key: 'Backspace' });
fireEvent.keyDown(chipDelete, { key: 'Enter' });
fireEvent.keyDown(chipDelete, { key: 'Delete' });
fireEvent.click(chipDelete);
expect(handleDelete.callCount).to.equal(4);
});

it('should not call onDelete function when ChipDelete is disabled', () => {
const handleDelete = spy();
const { getByRole } = render(
<ChipDelete disabled onDelete={handleDelete} onClick={() => {}} />,
);
const chipDelete = getByRole('button');
act(() => {
chipDelete.focus();
});
fireEvent.click(chipDelete);
expect(handleDelete.callCount).to.equal(0);
});
});
});
41 changes: 40 additions & 1 deletion packages/mui-joy/src/ChipDelete/ChipDelete.tsx
Expand Up @@ -75,6 +75,9 @@ const ChipDelete = React.forwardRef(function ChipDelete(inProps, ref) {
variant: variantProp,
color: colorProp,
disabled: disabledProp,
onKeyDown,
onDelete,
onClick,
...other
} = props;
const chipContext = React.useContext(ChipContext);
Expand All @@ -101,6 +104,27 @@ const ChipDelete = React.forwardRef(function ChipDelete(inProps, ref) {

const classes = useUtilityClasses(ownerState);

const handleClickDelete = (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => {
if (!disabled && onDelete) {
onDelete(event);
}
if (onClick) {
onClick(event);
}
};

const handleKeyDelete = (event: React.KeyboardEvent<HTMLButtonElement>) => {
event.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

What is the use case for preventDefault()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handleKeyDelete is firing twice for 1 keypress , e.preventDefault() is preventing that

Copy link
Member

Choose a reason for hiding this comment

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

Then it should be specific for those keys. Currently, TAB is stuck on the chip delete.

Screen.Recording.2565-12-15.at.11.35.18.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it so e.prevantDefault() should be inside if block, since you are committing to the pr will you make the change or should I.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, I can push the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I just pushed it. Hope it's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (['Backspace', 'Enter', 'Delete'].includes(event.key)) {
if (!disabled && onDelete) {
onDelete(event);
}
}
if (onKeyDown) {
onKeyDown(event);
}
};

const rootProps = useSlotProps({
elementType: ChipDeleteRoot,
getSlotProps: getRootProps,
Expand All @@ -109,11 +133,14 @@ const ChipDelete = React.forwardRef(function ChipDelete(inProps, ref) {
ownerState,
additionalProps: {
as: component,
onKeyDown: handleKeyDelete,
onClick: handleClickDelete,
},
className: classes.root,
});

return <ChipDeleteRoot {...rootProps}>{children ?? <Cancel />}</ChipDeleteRoot>;
const { onDelete: excludeOnDelete, ...restOfRootProps } = rootProps;
return <ChipDeleteRoot {...restOfRootProps}>{children ?? <Cancel />}</ChipDeleteRoot>;
}) as OverridableComponent<ChipDeleteTypeMap>;

ChipDelete.propTypes /* remove-proptypes */ = {
Expand Down Expand Up @@ -143,6 +170,18 @@ ChipDelete.propTypes /* remove-proptypes */ = {
* If `undefined`, the value inherits from the parent chip via a React context.
*/
disabled: PropTypes.bool,
/**
* @ignore
*/
onClick: PropTypes.func,
/**
* Callback fired when component is not disabled and `Backspace`, `Enter` or `Delete` is pressed or when chip is clicked.
*/
onDelete: PropTypes.func,
/**
* @ignore
*/
onKeyDown: PropTypes.func,
/**
* The system prop that allows defining system overrides as well as additional CSS styles.
*/
Expand Down
4 changes: 4 additions & 0 deletions packages/mui-joy/src/ChipDelete/ChipDeleteProps.ts
Expand Up @@ -23,6 +23,10 @@ export interface ChipDeleteTypeMap<P = {}, D extends React.ElementType = 'button
* If `undefined`, the value inherits from the parent chip via a React context.
*/
disabled?: boolean;
/**
* Callback fired when component is not disabled and `Backspace`, `Enter` or `Delete` is pressed or when chip is clicked.
*/
onDelete?: React.EventHandler<any>;
/**
* The system prop that allows defining system overrides as well as additional CSS styles.
*/
Expand Down