Skip to content

sol#1140

Open
Bohdan259 wants to merge 5 commits intomate-academy:masterfrom
Bohdan259:develop
Open

sol#1140
Bohdan259 wants to merge 5 commits intomate-academy:masterfrom
Bohdan259:develop

Conversation

@Bohdan259
Copy link
Copy Markdown

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Hello! You've built a solid foundation for the autocomplete component, and the core functionality is working well. However, I'm requesting a few changes to fully meet the project requirements.

Here are the main points to address:

  • The debounce delay is currently hardcoded at 3000ms. This needs to be a customizable prop on your component with a default value of 300ms.
  • The filtering logic incorrectly runs when the input contains only spaces. Please add a check to prevent this, as required by [CHECKLIST ITEM #1].
  • The state variable onselected in App.tsx should be renamed to something more descriptive like selectedPerson to follow the naming conventions outlined in [CHECKLIST ITEM #4].

Please address these points to complete the task. Keep up the great work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

isOpen,
setIsOpen,
setQuery,
setFocused,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The dropdown is set to be open by default. It should initially be closed and open only upon user interaction with the input field.

}) => {
const handleInput = useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
setQuery(event.target.value);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The debounce delay is hardcoded to 3000ms. The task requires a default delay of 300ms, and it should be customizable via a prop passed to the PeopleMenu component.

src/App.tsx Outdated
const [isOpen, setIsOpen] = useState(true);
const [isFocused, setIsFocused] = useState(false);
const [query, setQuery] = useState('');
const [onselected, setOnselected] = useState<Person | null>(null);
Copy link
Copy Markdown

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 regarding naming conventions. The state variable onselected should be named based on what it holds, for example selectedPerson. The on prefix is conventionally used for event handler props or functions.

src/App.tsx Outdated
<div className="dropdown-item" data-cy="suggestion-item">
<p className="has-text-danger">Elisabeth Haverbeke</p>
</div>
const applyQuery = useCallback(debounce(setAppliedQuery, 3000), []);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The debounce delay is hardcoded to 3000ms. The requirement is for a default of 300ms, and it should be customizable via a prop on your Autocomplete component. This implementation doesn't allow for customization and uses an incorrect delay.

src/App.tsx Outdated
<p className="has-text-link">Pieter de Decker</p>
</div>
const filterPeople = useMemo(() => {
return peopleFromServer.filter(person =>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 .filter() method is called even if appliedQuery contains only spaces. You should add a condition to handle this case before attempting to filter the list.

src/note.tsx Outdated
Comment on lines +1 to +74
import React from 'react';
import './App.scss';
import { peopleFromServer } from './data/people';

export const App: React.FC = () => {
const { name, born, died } = peopleFromServer[0];

return (
<div className="container">
<main className="section is-flex is-flex-direction-column">
<h1 className="title" data-cy="title">
{`${name} (${born} - ${died})`}
</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>
</main>
</div>
);
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file appears to be for notes or a static example and is not part of the final application logic. As such, it has not been reviewed for correctness against the task requirements. Please ensure you remove any unused files before final submission.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Great job addressing the previous feedback! The component is very close to being complete, but a few adjustments are needed to fully meet the requirements. Here are the key issues to address:

  • Default Prop Value: The task requires the delay prop to have a default value of 300ms. Please make the prop optional in the type definition (delay?: number) and set its default value inside the component, for example, during destructuring: ({ delay = 300, ... }).
  • Prop Naming Convention: The setSelected prop should be renamed to onSelected. This is a standard React convention for event handler props and is required by the task's checklist [CHECKLIST ITEM #4].
  • State Update on Selection: When a user selects a person, the query state is updated, but appliedQuery is not. This causes a bug where refocusing the input shows suggestions for the old query. Please ensure appliedQuery is also updated when an item is selected.

You're almost there! Addressing these points will make your component more robust and fully compliant.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +5 to 9
import { Person } from './types/Person';

export const App: React.FC = () => {
const { name, born, died } = peopleFromServer[0];
const [selectedPerson, setSelectedPerson] = useState<Person | null>(null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job defining the component's props. To fully align with the task requirements, a couple of adjustments are needed:

  1. The task requires the delay prop to have a default value of 300ms. Currently, it's a required prop. You can make it optional (delay?: number) and set its default value during destructuring in the component, like ({ delay = 300, ...rest }).

  2. The task description specifies that the callback should be named onSelected. Please rename the setSelected prop to onSelected to match the requirements and follow common React naming conventions for event handlers.

Comment on lines +6 to +28
delay: number;
people: Person[];
setSelected: (person: Person | null) => void;
};

function debounce<T extends unknown[]>(
callback: (...args: T) => void,
delay: number,
) {
let timerId = 0;

return (...args: T) => {
window.clearTimeout(timerId);
timerId = window.setTimeout(() => {
callback(...args);
}, delay);
};
}

const PeopleMenuComponent: React.FC<Props> = ({
people,
setSelected,
delay,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The delay prop is required, but the task specifies it should have a default value of 300ms. It's best practice to set this default inside the PeopleMenuComponent itself, which also makes the prop optional and the component more reusable.

You can achieve this by setting a default value during prop destructuring and updating the type definition:

type Props = {
  delay?: number; // Make prop optional
  // ...
};

const PeopleMenuComponent: React.FC<Props> = ({
  delay = 300, // Set default value
  // ...
}) => { /* ... */ }

Comment on lines +65 to +71
const handleClickInput = useCallback(
(person: Person) => {
setSelected(person);
setQuery(person.name);
setIsOpen(false);
},
[setSelected, setQuery, setIsOpen],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When a user selects a person, the query state is updated, but appliedQuery is not. This can lead to a confusing user experience. If the user selects an item, clicks away, and then clicks back into the input, the suggestions will be based on the old appliedQuery (what they were typing before selecting), not the value currently in the input.

To fix this, you should also update appliedQuery when an item is selected.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am approving your solution as an educational override so you can move forward, but there's a crucial concept with React hooks we need to address. You've done an excellent job implementing the core features and addressing the previous feedback!

The main issue is with missing dependencies in your hooks in src/Components/PeopleMenu.tsx. For instance:

  • The useCallback for your debounced function doesn't include the delay prop in its dependency array. This means if delay changes, the function will keep using the old value.
  • Similarly, the useMemo for filterPeople is missing people, so it won't recalculate when the list of people changes.

Remember, any prop or state value used inside a useCallback or useMemo must be added to its dependency array. Fixing this will make your component fully reactive and robust. Keep up the fantastic work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

const [query, setQuery] = useState('');
const [appliedQuery, setAppliedQuery] = useState('');

const applyQuery = useCallback(debounce(setAppliedQuery, delay), []);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The useCallback hook here has an empty dependency array, which means the applyQuery function is created only once when the component mounts. If the delay prop changes, this debounced function will continue to use the initial delay value. To fix this, you should include delay in the dependency array.

.toLocaleLowerCase()
.includes(appliedQuery.toLocaleLowerCase()),
);
}, [appliedQuery]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The filterPeople function depends on the people prop, but it's missing from the useMemo dependency array. If the people array changes, this memoized value will not be recalculated, and the component will continue to show suggestions from the old list. Please add people to the dependency array to ensure the filter re-runs when the list of people changes.

setAppliedQuery(person.name)
setIsOpen(false);
},
[onSelected, setQuery, setIsOpen],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The handleClickInput callback uses setAppliedQuery, but it's not included in the dependency array. While state setter functions from useState are stable and this might not cause a bug in this specific case, it's a good practice and a rule of hooks to include all dependencies. Please add setAppliedQuery to the array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants