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

fix(Search): fix event passed to onChange when clearing input (#12133) #12140

Merged
merged 6 commits into from
Oct 4, 2022
Merged

fix(Search): fix event passed to onChange when clearing input (#12133) #12140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ceffc91
fix(Search): fix event passed to `onChange` when clearing input (#12133)
gi Sep 23, 2022
970b11d
Merge branch 'main' into issue-12133/search-clear-click-onchange
gi Sep 23, 2022
b034bde
Merge branch 'main' into issue-12133/search-clear-click-onchange
gi Sep 23, 2022
552dc50
test(Search): update mock onchange to check only value
gi Sep 28, 2022
28360aa
fix(Search): use input element as target in onChange event on clear
gi Sep 28, 2022
5365473
Merge branch 'main' into issue-12133/search-clear-click-onchange
kodiakhq[bot] Oct 4, 2022
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
51 changes: 51 additions & 0 deletions packages/react/src/components/Search/next/Search-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,23 @@ import { mount, shallow } from 'enzyme';
const prefix = 'cds';

describe('Search', () => {
let wrapper;

// const container = () => wrapper.find(`.${prefix}--search`);
const button = () => wrapper.find('button');
const input = () => wrapper.find('input');
// const label = () => wrapper.find('label');

const render = (props) => {
if (wrapper) {
return wrapper.setProps(props);
}

wrapper = mount(<Search labelText="testlabel" {...props} />);

return wrapper;
};

Comment on lines +16 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in a beforeEach or beforeAll?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be a beforeAll, but that would only be doing the same thing as this but with less clarity: the functions would need to be declared as let variables and then assigned in the before routine.

This code solves for an issue that I see a lot with front-end tests and which I see with the tests in this suite: if you render the element under test in a beforeEach routine, then it makes any other setup in nested contexts very difficult. You end up with multiple renders of the element.

In fact, I believe that some of the tests in this suite are finding the wrong element rendered.

If you move the mounting or rendering into each test/it, then you avoid this issue.

To avoid duplicating the possibly complex mount/render declaration, I have adopted this paradigm which works quite well in my experience.

Each nested context/describe can build on the props:

  • default: props reset
    • with size large: props.large = true
    • with size small: props.small = true
  • with onChange: props.onChange = jest.fn();
  • etc.

Each test simply becomes

render(props);
expect(...);

describe('renders as expected', () => {
const wrapper = mount(
<Search
Expand Down Expand Up @@ -181,6 +198,40 @@ describe('Search', () => {
});

describe('events', () => {
describe('onChange', () => {
const onChange = jest.fn();

beforeEach(() => {
onChange.mockReset();
render({ onChange: (e) => onChange(e.target.value) });
});

describe('when input value is changed', () => {
const target = { value: 'test' };
const mock = { target };

beforeEach(() => {
input().simulate('change', mock);
});

it('is called', () => {
expect(onChange).toHaveBeenCalledWith(target.value);
});
});

describe('when clear button is clicked', () => {
const target = { value: '' };

beforeEach(() => {
button().simulate('click');
});

it('is called', () => {
expect(onChange).toHaveBeenCalledWith(target.value);
});
});
});

describe('enabled textinput', () => {
const onClick = jest.fn();
const onChange = jest.fn();
Expand Down
14 changes: 5 additions & 9 deletions packages/react/src/components/Search/next/Search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,15 @@ const Search = React.forwardRef(function Search(
setPrevValue(value);
}

function clearInput(event) {
function clearInput() {
if (!value) {
inputRef.current.value = '';
onChange(event);
} else {
const clearedEvt = Object.assign({}, event.target, {
target: {
value: '',
},
});
onChange(clearedEvt);
}

const inputTarget = Object.assign({}, inputRef.current, { value: '' });
const clearedEvt = { target: inputTarget, type: 'change' };

onChange(clearedEvt);
onClear();
setHasContent(false);
focus(inputRef);
Expand Down