-
Notifications
You must be signed in to change notification settings - Fork 858
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
solution #758
base: master
Are you sure you want to change the base?
solution #758
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,96 @@ | ||||||||||||||||
import React, { FC, useState, useCallback, useMemo } from 'react'; | ||||||||||||||||
import debounce from 'lodash.debounce'; | ||||||||||||||||
import cn from 'classnames'; | ||||||||||||||||
|
||||||||||||||||
import { peopleFromServer } from '../../data/people'; | ||||||||||||||||
import { Person } from '../../types/Person'; | ||||||||||||||||
import { filterPeople } from '../../utils/filterPeople'; | ||||||||||||||||
import { DropdownMenu } from '../DropdownMenu/DropdownMenu'; | ||||||||||||||||
|
||||||||||||||||
type Props = { | ||||||||||||||||
selectedPerson: Person | null; | ||||||||||||||||
onSelect: React.Dispatch<React.SetStateAction<Person | null>>; | ||||||||||||||||
delay?: number; | ||||||||||||||||
}; | ||||||||||||||||
|
||||||||||||||||
export const Dropdown: FC<Props> = ({ | ||||||||||||||||
selectedPerson, | ||||||||||||||||
onSelect, | ||||||||||||||||
delay = 300, | ||||||||||||||||
}) => { | ||||||||||||||||
const [query, setQuery] = useState(''); | ||||||||||||||||
const [appliedQuery, setAppliedQuery] = useState(''); | ||||||||||||||||
const [isActive, setIsActive] = useState(false); | ||||||||||||||||
|
||||||||||||||||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||||||||||||||||
const applyQuery = useCallback(debounce(setAppliedQuery, delay), []); | ||||||||||||||||
|
||||||||||||||||
const filteredPeople = useMemo( | ||||||||||||||||
() => filterPeople(peopleFromServer, { appliedQuery }), | ||||||||||||||||
[appliedQuery], | ||||||||||||||||
); | ||||||||||||||||
Comment on lines
+28
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. change syntax and use it in the future(more readable)
Suggested change
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. I'll remember it, thanks, but here prettier changes it back |
||||||||||||||||
|
||||||||||||||||
const visiblePeople = selectedPerson ? peopleFromServer : filteredPeople; | ||||||||||||||||
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 key for list items is generated during render which is not a good practice. As per the article, it's better to generate unique keys for list items when the list is created. 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. useMemo? |
||||||||||||||||
|
||||||||||||||||
const handleInputFocus = () => { | ||||||||||||||||
setIsActive(true); | ||||||||||||||||
}; | ||||||||||||||||
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 |
||||||||||||||||
|
||||||||||||||||
const handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||||||||||||||||
setQuery(event.target.value.trimStart()); | ||||||||||||||||
applyQuery(event.target.value); | ||||||||||||||||
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. You need to make sure that filter won't be called if user entered spaces only. In the current implementation, if the user only enters spaces, the filter function will still be called. You should trim the input value before applying the filter. |
||||||||||||||||
onSelect(null); | ||||||||||||||||
}; | ||||||||||||||||
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 |
||||||||||||||||
|
||||||||||||||||
const handleSelect = (person: Person) => { | ||||||||||||||||
onSelect(person); | ||||||||||||||||
setQuery(person.name); | ||||||||||||||||
setIsActive(false); | ||||||||||||||||
}; | ||||||||||||||||
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 |
||||||||||||||||
|
||||||||||||||||
return ( | ||||||||||||||||
<> | ||||||||||||||||
<div | ||||||||||||||||
className={cn('dropdown', { | ||||||||||||||||
'is-active': isActive, | ||||||||||||||||
})} | ||||||||||||||||
> | ||||||||||||||||
<div className="dropdown-trigger"> | ||||||||||||||||
<input | ||||||||||||||||
type="text" | ||||||||||||||||
placeholder="Enter a part of the name" | ||||||||||||||||
data-cy="search-input" | ||||||||||||||||
className="input" | ||||||||||||||||
value={query} | ||||||||||||||||
onFocus={handleInputFocus} | ||||||||||||||||
onChange={handleInputChange} | ||||||||||||||||
/> | ||||||||||||||||
</div> | ||||||||||||||||
|
||||||||||||||||
{!!filteredPeople.length && ( | ||||||||||||||||
<DropdownMenu | ||||||||||||||||
people={visiblePeople} | ||||||||||||||||
selectedPerson={selectedPerson} | ||||||||||||||||
onSelect={handleSelect} | ||||||||||||||||
/> | ||||||||||||||||
)} | ||||||||||||||||
</div> | ||||||||||||||||
|
||||||||||||||||
{!filteredPeople.length && ( | ||||||||||||||||
<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> | ||||||||||||||||
)} | ||||||||||||||||
</> | ||||||||||||||||
); | ||||||||||||||||
}; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,6 @@ | ||||||
@import '../../variables/colors'; | ||||||
|
||||||
.dropdown-item:hover { | ||||||
background-color: $dropdown-backgrount-hover-color; | ||||||
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. syntax mistake
Suggested change
|
||||||
cursor: pointer; | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import { FC } from 'react'; | ||
import cn from 'classnames'; | ||
import './DropdownMenu.scss'; | ||
import { Person } from '../../types/Person'; | ||
|
||
type Props = { | ||
people: Person[]; | ||
selectedPerson: Person | null; | ||
onSelect: (person: Person) => void; | ||
}; | ||
|
||
export const DropdownMenu: FC<Props> = ({ | ||
people, | ||
selectedPerson, | ||
onSelect, | ||
}) => { | ||
return ( | ||
<div className="dropdown-menu" role="menu" data-cy="suggestions-list"> | ||
<div className="dropdown-content"> | ||
{people.map(person => { | ||
const { name, sex } = person; | ||
|
||
return ( | ||
<div | ||
key={person.slug} | ||
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. Key generation on render is not recommended. This is because keys should be stable between multiple renders for performance reasons. You can read more about it here. |
||
className={cn('dropdown-item', { | ||
'has-background-grey-lighter': selectedPerson === person, | ||
})} | ||
data-cy="suggestion-item" | ||
onClick={() => onSelect(person)} | ||
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 is a performance issue. You are creating a new function every time the component renders. This is not recommended. Instead, create a function inside the component that accepts a person as an argument and call this function in your onClick handler. This way, a new function is not created on every render. 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 naming convention for event handlers in React suggests that the name should start with 'handle' and end with the type of event you are handling. In this case, a better name would be 'handleSelectPerson'. You can read more about the naming conventions here. |
||
> | ||
<p | ||
className={cn({ | ||
'has-text-link': sex === 'm', | ||
'has-text-danger': sex === 'f', | ||
})} | ||
> | ||
{name} | ||
</p> | ||
</div> | ||
); | ||
})} | ||
</div> | ||
</div> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { Person } from '../types/Person'; | ||
|
||
export function filterPeople( | ||
people: Person[], | ||
{ appliedQuery }: { appliedQuery: string }, | ||
) { | ||
let filteredPeople = [...people]; | ||
|
||
if (appliedQuery) { | ||
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. You should check if |
||
const normalizedQuery = appliedQuery.toLocaleLowerCase().trim(); | ||
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 |
||
|
||
filteredPeople = filteredPeople.filter(person => | ||
person.name.toLocaleLowerCase().includes(normalizedQuery), | ||
); | ||
} | ||
|
||
return filteredPeople; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
$dropdown-backgrount-hover-color: hsl(0, 0%, 96%); |
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.
I think - this will be better and more readable, right?