Solution #1166
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,73 +1,36 @@ | ||
| import React, { useState } from 'react'; | ||
| import './App.scss'; | ||
| import { peopleFromServer } from './data/people'; | ||
| import { Autocomplete } from './Autocomplete'; | ||
| import { Person } from './types/Person'; | ||
|
|
||
| export const App: React.FC = () => { | ||
| const [selectedPerson, setSelectedPerson] = useState<Person | null>(null); | ||
| const [query, setQuery] = useState(''); | ||
| const delayValue = 300; | ||
|
|
||
| return ( | ||
| <div className="container"> | ||
| <main className="section is-flex is-flex-direction-column"> | ||
| <h1 className="title" data-cy="title"> | ||
| {selectedPerson | ||
| ? `${selectedPerson.name} (${selectedPerson.born} - ${selectedPerson.died})` | ||
| : 'No selected person'} | ||
| </h1> | ||
|
|
||
| <Autocomplete | ||
| people={peopleFromServer} | ||
| onSelected={person => { | ||
| setSelectedPerson(person); | ||
| setQuery(person.name); | ||
| }} | ||
| delay={delayValue} | ||
| query={query} | ||
| onQueryChange={value => { | ||
| setQuery(value); | ||
| setSelectedPerson(null); | ||
|
Comment on lines
+24
to
+31
There was a problem hiding this comment. Checklist item #4 violation: Function name |
||
| }} | ||
| /> | ||
| </main> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| import { useCallback, useRef, useState } from 'react'; | ||
| import { Person } from './types/Person'; | ||
| import debounce from 'lodash.debounce'; | ||
| import classNames from 'classnames'; | ||
|
|
||
| type Props = { | ||
| people: Person[]; | ||
| onSelected: (person: Person) => void; | ||
| delay?: number; | ||
| query: string; | ||
| onQueryChange: (value: string) => void; | ||
| }; | ||
| export const Autocomplete: React.FC<Props> = ({ | ||
| people, | ||
| onSelected, | ||
| delay, | ||
| query, | ||
| onQueryChange, | ||
| }) => { | ||
| const [appliedQuery, setAppliedQuery] = useState(''); | ||
| const [isOpen, setIsOpen] = useState(false); | ||
|
|
||
| const lastAppliedQuery = useRef(''); | ||
|
|
||
| const isAppliedQuery = (value: string) => { | ||
| if (lastAppliedQuery.current === value) { | ||
| return; | ||
| } | ||
|
|
||
| lastAppliedQuery.current = value; | ||
| setAppliedQuery(value); | ||
|
Comment on lines
+24
to
+31
There was a problem hiding this comment. This function name implies it returns a boolean (
Comment on lines
+24
to
+31
There was a problem hiding this comment. This violates checklist item #4 'follow naming conventions for methods'. Function name |
||
| }; | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const applyQuery = useCallback(debounce(isAppliedQuery, delay), [delay]); | ||
|
Comment on lines
+34
to
+35
There was a problem hiding this comment. The debounced function should be cancelled on unmount to prevent memory leaks. Add a useEffect cleanup: |
||
|
|
||
| const handleQueryChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| onQueryChange(event.target.value); | ||
| applyQuery(event.target.value); | ||
| }; | ||
|
|
||
| const normalizedQuery = appliedQuery.trim().toLowerCase(); | ||
|
|
||
| const filteredPeople = appliedQuery | ||
| ? people.filter(person => | ||
| person.name.toLowerCase().includes(normalizedQuery)) | ||
|
There was a problem hiding this comment. This violates checklist item #4: 'make sure that filter won't be called if user entered spaces only'. When |
||
| : people; | ||
|
|
||
| return ( | ||
| <div | ||
| className={classNames('dropdown', { | ||
| 'is-active': isOpen, | ||
| })} | ||
| > | ||
| <div className="dropdown-trigger"> | ||
| <input | ||
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" | ||
| value={query} | ||
| onChange={e => { | ||
| handleQueryChange(e); | ||
|
Comment on lines
+68
to
+70
There was a problem hiding this comment. Redundant wrapper - can be simplified to
Comment on lines
+68
to
+70
There was a problem hiding this comment. This violates checklist item #4 - redundant wrapper. Change |
||
| }} | ||
| onFocus={() => { | ||
| setIsOpen(true); | ||
| }} | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="dropdown-menu" role="menu" data-cy="suggestions-list"> | ||
| <div className="dropdown-content"> | ||
| {filteredPeople.map(person => ( | ||
| <div | ||
| key={person.slug} | ||
| className="dropdown-item" | ||
| data-cy="suggestion-item" | ||
| onClick={() => { | ||
| onSelected(person); | ||
| setIsOpen(false); | ||
| }} | ||
| > | ||
| <p className="has-text-link">{person.name}</p> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
|
|
||
| {isOpen && appliedQuery && filteredPeople.length === 0 && ( | ||
| <div | ||
| className=" | ||
| notification | ||
| is-danger | ||
| is-light | ||
| mt-3 | ||
| is-align-self-flex-start" | ||
| role="alert" | ||
| data-cy="no-suggestions-message" | ||
| > | ||
| <p className="has-text-danger">No matching suggestions</p> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This violates checklist item #4: 'follow naming conventions for methods'. The function name
isAppliedQueryimplies it returns a boolean, but it returnsundefinedwith side effects. Rename it to something action-oriented likeapplyQueryIfChanged.