Skip to content

changed the searchbar to functional #2431

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

Merged
merged 4 commits into from
Jun 13, 2024
Merged

changed the searchbar to functional #2431

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f900d6c
changed the searchbar to functional
VarunMotiyani Sep 13, 2023
8a99f23
Merge branch 'develop' into main
raclim Mar 8, 2024
aa791fa
Merge branch 'develop' into main
VarunMotiyani Mar 14, 2024
838071d
Merge branch 'develop' into main
raclim Jun 13, 2024
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
100 changes: 47 additions & 53 deletions client/modules/IDE/components/Searchbar/Searchbar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,71 +1,65 @@
import React, { useState, useCallback, useEffect } from 'react';
import PropTypes from 'prop-types';
import React from 'react';
import { throttle } from 'lodash';
import { withTranslation } from 'react-i18next';
import i18next from 'i18next';
import SearchIcon from '../../../../images/magnifyingglass.svg';

class Searchbar extends React.Component {
constructor(props) {
super(props);
this.state = {
searchValue: this.props.searchTerm
};
this.throttledSearchChange = throttle(this.searchChange, 500);
}
const Searchbar = ({
searchTerm,
setSearchTerm,
resetSearchTerm,
searchLabel,
t
}) => {
const [searchValue, setSearchValue] = useState(searchTerm);

componentWillUnmount() {
this.props.resetSearchTerm();
}
const throttledSearchChange = useCallback(
throttle((value) => {
setSearchTerm(value.trim());
}, 500),
[]
);

handleResetSearch = () => {
this.setState({ searchValue: '' }, () => {
this.props.resetSearchTerm();
});
const handleResetSearch = () => {
setSearchValue('');
resetSearchTerm();
};

searchChange = () => {
this.props.setSearchTerm(this.state.searchValue.trim());
const handleSearchChange = (e) => {
const { value } = e.target;
setSearchValue(value);
throttledSearchChange(value.trim());
};

handleSearchChange = (e) => {
this.setState({ searchValue: e.target.value }, () => {
this.throttledSearchChange(this.state.searchValue.trim());
});
};
useEffect(() => {
setSearchValue(searchTerm);
}, [searchTerm]);
Comment on lines +35 to +37
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really thinking through this effect as I go back and forth on whether it's good or bad. You're the only one who thought to include this here are we didn't have this logic in the class component version. It definitely shows a lot of foresight on your part to think about this 👍.

The situation where the searchTerm props changes based on some external factor doesn't come up in practice with the way that we are currently using the Searchbar component, as all changes are initiated by the component itself. But I'm all about separation of concerns and designing flexible and reusable components. The current component has the limitation that it will ignore any changes to searchTerm after mount and you've removed that limitation.

The main reason that I'm hesitating is a fear that this might interact badly with the throttle. I constantly have to remind myself of the difference between debounce and throttle so maybe it's not an issue. I'm concerned that there might be some pending changes which haven't been submitted at the time that searchTerm prop gets updated. Like if I'm trying to type "abc def" and the throttled submit goes through with "abc" but in the meantime I've typed " def" but now it's reset back to "abc". I'm not sure if that could happen.


render() {
const { searchValue } = this.state;
return (
<div
className={`searchbar ${
searchValue === '' ? 'searchbar--is-empty' : ''
}`}
>
<div className="searchbar__button">
<SearchIcon
className="searchbar__icon"
focusable="false"
aria-hidden="true"
/>
</div>
<input
className="searchbar__input"
type="text"
value={searchValue}
placeholder={this.props.searchLabel}
onChange={this.handleSearchChange}
return (
<div
className={`searchbar ${searchValue === '' ? 'searchbar--is-empty' : ''}`}
>
<div className="searchbar__button">
<SearchIcon
className="searchbar__icon"
focusable="false"
aria-hidden="true"
/>
<button
className="searchbar__clear-button"
onClick={this.handleResetSearch}
>
{this.props.t('Searchbar.ClearTerm')}
</button>
</div>
);
}
}
<input
className="searchbar__input"
type="text"
value={searchValue}
placeholder={searchLabel}
onChange={handleSearchChange}
/>
<button className="searchbar__clear-button" onClick={handleResetSearch}>
{t('Searchbar.ClearTerm')}
</button>
</div>
);
};

Searchbar.propTypes = {
searchTerm: PropTypes.string.isRequired,
Expand Down
Loading