-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Solution #1166
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
base: master
Are you sure you want to change the base?
Solution #1166
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,73 +1,36 @@ | ||
| import React from 'react'; | ||
| 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 { name, born, died } = peopleFromServer[0]; | ||
| 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"> | ||
| {`${name} (${born} - ${died})`} | ||
| {selectedPerson | ||
| ? `${selectedPerson.name} (${selectedPerson.born} - ${selectedPerson.died})` | ||
| : 'No selected person'} | ||
| </h1> | ||
|
|
||
| <div className="dropdown is-active"> | ||
| <div className="dropdown-trigger"> | ||
| <input | ||
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="dropdown-menu" role="menu" data-cy="suggestions-list"> | ||
| <div className="dropdown-content"> | ||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-link">Pieter Haverbeke</p> | ||
| </div> | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-link">Pieter Bernard Haverbeke</p> | ||
| </div> | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-link">Pieter Antone Haverbeke</p> | ||
| </div> | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-danger">Elisabeth Haverbeke</p> | ||
| </div> | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-link">Pieter de Decker</p> | ||
| </div> | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-danger">Petronella de Decker</p> | ||
| </div> | ||
|
|
||
| <div className="dropdown-item" data-cy="suggestion-item"> | ||
| <p className="has-text-danger">Elisabeth Hercke</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <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> | ||
| <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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checklist item #4 violation: Function name |
||
| }} | ||
| /> | ||
| </main> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import { useCallback, useEffect, 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function name implies it returns a boolean (
Comment on lines
+24
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The debounced function should be cancelled on unmount to prevent memory leaks. Add a useEffect cleanup: |
||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| applyQuery.cancel(); | ||
| }; | ||
| }, [applyQuery]); | ||
|
|
||
| const handleQueryChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| onQueryChange(event.target.value); | ||
| applyQuery(event.target.value); | ||
| }; | ||
|
|
||
| const normalizedQuery = appliedQuery.trim().toLowerCase(); | ||
|
|
||
| const filteredPeople = normalizedQuery | ||
| ? people.filter(person => | ||
| person.name.toLowerCase().includes(normalizedQuery), | ||
| ) | ||
| : 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant wrapper - can be simplified to
Comment on lines
+68
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.