Skip to content

Solved task#1127

Open
AndreyKagaml wants to merge 3 commits intomate-academy:masterfrom
AndreyKagaml:develop
Open

Solved task#1127
AndreyKagaml wants to merge 3 commits intomate-academy:masterfrom
AndreyKagaml:develop

Conversation

@AndreyKagaml
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

You've built a good foundation with a working debounced filter, which is a great start. However, this submission requires changes because some core functionality is missing.

Here are the main issues to address:

  • Selection logic is incomplete: In App.tsx, when a user clicks a suggestion, the input field's text is not updated and the suggestion list does not close. You need to create a new handler for the onSelected prop that updates the query state and hides the list.
  • Whitespace filtering: The filter runs even for input containing only spaces, which violates a checklist requirement [CHECKLIST ITEM #1]. Please make sure to .trim() the user's input before filtering.
  • Incorrect message visibility: The "No matching suggestions" message remains visible even when the dropdown is closed. Its display should be conditional on the dropdown being open and the query having no results.

✨ 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

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 some of the previous feedback! While the foundation is strong, I'm requesting changes because a few critical requirements are not yet met, which break the core functionality of the autocomplete component.

Here’s what needs to be fixed:

  1. Selection Logic: When a suggestion is clicked, the input field is cleared instead of being filled with the selected name. In your handleSelect function, please set the query state to the person's name.
  2. Inability to Type Spaces: You are trimming the input value in handleQueryChange, which prevents users from typing spaces to search for multi-word names. The query state should reflect the user's raw input; trimming should only be applied to the value used for filtering.
  3. Customizable Debounce Delay: The task requires the debounce delay to be a customizable prop, but it is currently hardcoded. This logic should be part of a reusable component that accepts props.
  4. Incorrect Test Attributes: The data-qa attributes, which are required for tests, have been changed to data-cy. Please change them back to data-qa.

Once these issues are resolved, your component will be fully functional. 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

[preparedQuery],
);

const applyQuery = useCallback(debounce(setPreparedQuery, 300), []);
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 task requires the debounce delay to be customizable via a prop with a default value of 300ms. Currently, the delay is hardcoded here. This suggests that the autocomplete logic should be encapsulated in a separate, reusable component that can accept props.

return (
<div className="container">
<main className="section is-flex is-flex-direction-column">
<h1 className="title" data-cy="title">
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 task description explicitly states, "Don't remove the data-qa attributes. It is required for tests." All data-cy attributes throughout the application should be changed back to data-qa to ensure that the automated tests can run successfully.

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'm approving this submission as an educational override to help you progress. You've done a great job fixing the selection logic! However, a few key requirements still need attention.

Please make sure to:

  1. Use data-qa for test attributes instead of data-cy across all components, as required by the task.
  2. Make the debounce delay a customizable prop in App.tsx instead of hardcoding it.
  3. Prevent the filter from running when the input contains only spaces by checking the trimmed value. This addresses checklist item #1.
  4. Fix the focus/blur bug by canceling the debounced onBlur call inside the onFocus handler.

You're very close to a perfect solution. Keep up the fantastic effort!


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

Footnotes

  1. Rate AI review example

//const field = useRef<HTMLInputElement>(null);

return (
<div className="dropdown-menu" role="menu" data-cy="suggestions-list">
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 task requirements specify using data-qa attributes for testing, not data-cy. Please update this attribute to data-qa="suggestions-list".

{filteredPeople.map((person: Person) => (
<div
className="dropdown-item"
data-cy="suggestion-item"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As per the task requirements, this should be a data-qa attribute for testing purposes. Please change it to data-qa="suggestion-item".

Comment on lines +18 to +19
peopleFromServer.filter(person =>
person.name.includes(preparedQuery.trim()),
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 for an empty or spaces-only query. Consider adding a check to avoid filtering in this case and directly return the full list of people.

[preparedQuery],
);

const applyQuery = useCallback(debounce(setPreparedQuery, 300), []);
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. According to the requirements, the delay should be a customizable prop with a default value of 300ms.

return (
<div className="container">
<main className="section is-flex is-flex-direction-column">
<h1 className="title" data-cy="title">
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 task requires using data-qa attributes for testing, but data-cy is used here. This was also pointed out in the previous review. Please update all test attributes to data-qa.

@@ -19,55 +50,36 @@ export const App: React.FC = () => {
placeholder="Enter a part of the name"
className="input"
data-cy="search-input"
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 task requires using data-qa attributes for testing, but data-cy is used here.

Comment on lines +55 to +56
onFocus={() => setBlured(true)}
onBlur={() => loseQuery(false)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's a subtle bug in the focus/blur handling. If you blur the input and then quickly re-focus it (within 1 second), the suggestions will appear and then disappear. This is because the debounced loseQuery call from onBlur isn't cancelled. You can fix this by calling loseQuery.cancel() inside the onFocus handler.

is-align-self-flex-start
"
role="alert"
data-cy="no-suggestions-message"
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 task requires using data-qa attributes for testing, but data-cy is used here.

Copy link
Copy Markdown

@brespect brespect left a comment

Choose a reason for hiding this comment

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

Almost done, but make sure you have correctly deployed your DEMO LINK. For now, it is not working. Also, remove all redundant comments in final code

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.

3 participants