Skip to content
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

Develop #1084

Open
wants to merge 75 commits into
base: develop
Choose a base branch
from
Open

Develop #1084

wants to merge 75 commits into from

Conversation

CamilleHaus
Copy link

mgrinko and others added 30 commits July 4, 2019 09:14
stylelint was added to npm run lint command
the explanation of the task looked confusing to me + saving new todo on blur is quite weird behavior
Copy link

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


Thank you for your hard work on this project! 😊 However, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Component Splitting: The App.tsx file should be refactored to split the code into several components such as Filter, TodoList, Todo, and NewTodo. This will improve the maintainability and readability of your code.

  2. Unique ID for Todos: Ensure that each todo has a unique ID, potentially using +new Date() as suggested. This is crucial for managing todos effectively.

  3. Toggle All Button Behavior: The "Toggle all" button should only be active when all todos are completed. Currently, it is marked as active by default, which does not reflect the correct state.

  4. NewTodo Form Constraints: The NewTodo form should not allow the creation of empty todos and should trim redundant spaces. This will prevent invalid entries in your todo list.

  5. Use of Constants: Avoid using unknown strings directly in your code. Instead, define and use constants for filters to enhance code clarity and reduce errors.

  6. React Context: Implement React Context to manage todos, as this is a requirement of the task and will help in managing state more effectively.

  7. LocalStorage: Ensure that todos are saved to localStorage using JSON.stringify after each change. This functionality should be reflected in your main application code.

  8. Inline Editing: Implement the inline editing feature with functionalities such as saving changes on form submission, trimming the saved text, deleting the todo if the title is empty, saving changes on blur, and canceling editing on pressing Escape.

Please address these issues to meet the task requirements. Once these changes are made, we can proceed with the approval. Keep up the great work, and don't hesitate to reach out if you need any help! 🚀

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

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

todosCounter: () => cy.byDataCy('TodosCounter'),
clearCompletedButton: () => cy.byDataCy('ClearCompletedButton'),
footer: () => cy.byDataCy('Footer'),
localStorage: () => cy.getAllLocalStorage().its('http://localhost:3001'),

Choose a reason for hiding this comment

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

The use of cy.getAllLocalStorage().its('http://localhost:3001') assumes that the local storage is structured with this specific key. Ensure that this is consistent with your application's local storage setup, as it might not work if the base URL or storage key changes.

Comment on lines +50 to +51
Cypress.Commands.add('getByDataCy', selector => {
cy.get(`[data-cy="${selector}"]`);

Choose a reason for hiding this comment

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

The getByDataCy command should return the result of cy.get to ensure it can be chained properly. Modify it to return cy.get([data-cy="${selector}"]);.

{/* this button should have `active` class only if all todos are completed */}
<button
type="button"
className="todoapp__toggle-all active"

Choose a reason for hiding this comment

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

The className for ToggleAllButton includes active by default. Ensure this class is conditionally applied based on whether all todos are completed, as per the comment on line 11.

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.