Skip to content

Commit

Permalink
fix: Move SearchInput and SideNav callbacks outside of the setState c…
Browse files Browse the repository at this point in the history
…allback to maintain event properties (#1165)

BREAKING CHANGE: If consumers had used the SearchInput expecting the input's ref value to be updated inside of the callback, this is a breaking change, but that seems like an atypical, non-react way to handle changes
  • Loading branch information
sam-kvale-sap authored Aug 17, 2020
1 parent 5c95f43 commit e1127b1
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 15 deletions.
8 changes: 3 additions & 5 deletions src/SearchInput/SearchInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,9 @@ class SearchInput extends PureComponent {
value: item.text,
isExpanded: false,
searchExpanded: false
}, () => {
item?.callback();
this.props.onSelect(event, item);
});
item?.callback();
this.props.onSelect(event, item);
};

handleChange = event => {
Expand All @@ -52,9 +51,8 @@ class SearchInput extends PureComponent {
this.setState({
value: event.target.value,
isExpanded: true
}, () => {
this.props.onChange(event, filteredResult);
});
this.props.onChange(event, filteredResult);
};

handleClick = () => {
Expand Down
8 changes: 3 additions & 5 deletions src/SearchInput/SearchInput.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,24 +267,22 @@ describe('<SearchInput />', () => {
});

test('should allow props list to be changed after creation', () => {
let ref;
class Test extends React.Component {
constructor(props) {
super(props);
ref = React.createRef();
this.state = {
list: searchData
};
}

handleChange = () => {
if (ref.current.value === 'pe') {
handleChange = (e) => {
if (e.target.value === 'pe') {
this.setState({
list: searchDataNew
});
}
}
render = () => (<SearchInput inputProps={{ ref: ref }} onChange={this.handleChange}
render = () => (<SearchInput onChange={this.handleChange}
searchList={this.state.list} />);
}
const wrapper = mount(<Test />);
Expand Down
3 changes: 1 addition & 2 deletions src/SideNavigation/SideNav.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,8 @@ class SideNav extends Component {
handleSelect = (e, id) => {
this.setState({
selectedId: id
}, () => {
this.props.onItemSelect(e, id);
});
this.props.onItemSelect(e, id);
}

render() {
Expand Down
5 changes: 2 additions & 3 deletions src/SideNavigation/SideNav.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ describe('<SideNav />', () => {
describe('onItemSelect handler', () => {
test('should dispatch the onItemSelect callback with the event', () => {
let f = jest.fn();
const mockedEvent = { target: {} };

const element = mount(<SideNav
data-sample='Sample'
Expand All @@ -148,10 +147,10 @@ describe('<SideNav />', () => {
</SideNav.List>
</SideNav>);

element.find('#item-1 a').simulate('click', mockedEvent);
element.find('#item-1 a').simulate('click');

expect(f).toHaveBeenCalledTimes(1);
expect(f).toHaveBeenCalledWith(expect.objectContaining({ 'target': {} }), 'item-1');
expect(f).toHaveBeenCalledWith(expect.objectContaining({ 'target': expect.anything() }), 'item-1');
});
});
});

0 comments on commit e1127b1

Please sign in to comment.