-
Notifications
You must be signed in to change notification settings - Fork 1.3k
solution #1158
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 #1158
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 |
|---|---|---|
| @@ -1,73 +1,93 @@ | ||
| import React from 'react'; | ||
| import './App.scss'; | ||
| import React, { useEffect, useState } from 'react'; | ||
| import { PeopleList } from './components/peopleList'; | ||
| import { useFilter } from './hooks/Filter'; | ||
| import { Person } from './types/Person'; | ||
| import { peopleFromServer } from './data/people'; | ||
|
|
||
| export const App: React.FC = () => { | ||
| const { name, born, died } = peopleFromServer[0]; | ||
| const [query, setQuery] = useState(''); | ||
| const [data] = useState<Person[]>(peopleFromServer); | ||
| const [error, setError] = useState(false); | ||
| const [selectedPerson, setSelectedPerson] = useState<Person | null>(null); | ||
| const [show, setShow] = useState(false); | ||
|
|
||
| const filteredPeople = useFilter(data, query); | ||
|
|
||
| useEffect(() => { | ||
| // скрываем список после выбора | ||
| if (selectedPerson && show) { | ||
| setShow(false); | ||
| } | ||
|
|
||
| // показываем ошибку только когда dropdown открыт | ||
| if (show && query.trim() !== '' && filteredPeople.length === 0) { | ||
| setError(true); | ||
| } else { | ||
| setError(false); | ||
| } | ||
| }, [filteredPeople, query, selectedPerson, show]); | ||
|
|
||
| const handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| setQuery(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. This violates checklist item #1: "make sure that filter won't be called if user enred spaces only". The current check in useFilter ( |
||
| setSelectedPerson(null); | ||
| setShow(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. This may violate checklist item #4 (naming conventions). Event handler props are named |
||
|
|
||
| const handleSelect = (person: Person) => { | ||
| setSelectedPerson(person); | ||
| setQuery(person.name); // подставляем имя в input | ||
| }; | ||
|
|
||
| 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> | ||
|
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: "don't generate key on render" — in peopleList.tsx the list uses |
||
|
|
||
| <div className="dropdown is-active"> | ||
| <div className={`dropdown ${show ? 'is-active' : ''}`}> | ||
| <div className="dropdown-trigger"> | ||
| <input | ||
| type="text" | ||
| placeholder="Enter a part of the name" | ||
| className="input" | ||
| data-cy="search-input" | ||
| value={query} | ||
| onChange={handleInputChange} | ||
| onFocus={() => setShow(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. This violates checklist item #2: "don't interact with DOM directly, use React as much as possible." I don't see direct DOM interactions in these files, but App currently uses |
||
| /> | ||
| </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> | ||
| {show && ( | ||
| <div | ||
| className="dropdown-menu" | ||
| role="menu" | ||
| data-cy="suggestions-list" | ||
| > | ||
| <div className="dropdown-content"> | ||
| <PeopleList people={filteredPeople} onSelect={handleSelect} /> | ||
| </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> | ||
| {error && ( | ||
| <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> | ||
| )} | ||
| </main> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import type { Person } from '../types/Person'; | ||
|
|
||
| interface PeopleListProps { | ||
|
Comment on lines
+1
to
+3
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: "Don't interact with DOM directly, use React as much as possible" — code uses React state and props, no direct DOM manipulation found. Satisfied. |
||
| people: Person[]; | ||
| onSelect?: (person: Person) => void; | ||
| } | ||
|
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 file doesn't prevent filtering when the input contains only spaces. The checklist requires: "make sure that filter won't be called if user enred spaces only". In |
||
|
|
||
| export const PeopleList = ({ | ||
| people, | ||
| onSelect = () => {}, | ||
| }: PeopleListProps) => { | ||
| return ( | ||
| <> | ||
| {people.map(person => ( | ||
|
Comment on lines
+1
to
+14
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 task's debounce requirement and customizable |
||
| <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. Checklist item: "Don't generate key on render" — PeopleList uses 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: "don't generate key on render" is satisfied (you use stable |
||
| className="dropdown-item has-text-link is-clickable" | ||
| data-cy="suggestion-item" | ||
| onClick={() => onSelect(person)} | ||
| > | ||
| {person.name} | ||
| </div> | ||
| ))} | ||
|
Comment on lines
+22
to
+23
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. Requirement: "Show |
||
| </> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { useMemo } from 'react'; | ||
|
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 #2: 'don't interact with DOM directly, use React as much as possible'. The implementation doesn't interact with the DOM directly, so no issue here (no direct DOM access found). |
||
| import { Person } from '../types/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. Missing requirement: 'Implement an |
||
|
|
||
|
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. Requirement: 'The |
||
| export const useFilter = (people: Person[], query: string) => { | ||
| return useMemo(() => { | ||
| if (!query.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. This violates checklist item #1: 'make sure that filter won't be called if user enred spaces only'. The current filter hook calls |
||
| return people; // 👈 show all when empty | ||
| } | ||
|
|
||
| return people.filter(person => | ||
| person.name.toLowerCase().includes(query.toLowerCase()), | ||
| ); | ||
| }, [people, query]); | ||
| }; | ||
|
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. Missing debounce requirement: The description requires suggestions appear after some 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. Missing requirement: 'Don't run filtering again if the text has not changed (a pause in typing happened when the text was the same as before)'. There's no logic preventing repeated filtering on identical query pauses — currently useFilter depends on |
||
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 #3: "don't generate key on render." In PeopleList you use
key={person.slug}which is stable and acceptable. No dynamic/generated key on render is present, so no change needed for keys.