Skip to content

Spots final#2

Open
srulapaugh wants to merge 3 commits into
mainfrom
spots-final
Open

Spots final#2
srulapaugh wants to merge 3 commits into
mainfrom
spots-final

Conversation

@srulapaugh

Copy link
Copy Markdown
Owner

No description provided.

@sebasLTripleTen sebasLTripleTen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hello, Sam!

Thank you for submitting your project. It's clear that a lot of work has been put into this project.

Parts I really liked in your project:

  • Well-structured and clean code
  • The code for closing the modal windows is executed in the right place
  • Great job handling potential errors at the end of your server requests.
  • It's good that you're using finally blocks to reset the button text.

There are few things that need to be corrected in your project. They're mostly minor issues that are easy to fix.

  • All comments in the code, along with the ones below:
    • The code that checks the server response should be refactored to avoid repetition.
    • You need to disable the submit button immediately after the form is reset.
    • The resetValidation method should be called only when the Edit Profile form is opened.
    • The submit button should only be disabled after receiving a successful response from the server.

There are also some comments for improvement marked as "Could be improved". They don’t mean that you’ve done anything wrong, but they’re things we wanted to point out that can help you further develop your skills.

Reminder: For the project to be accepted, you must fix all "Needs correcting" comments. "Could be improved" comments are optional, so you can implement them if you choose.

It's fantastic to see how far you've come with this project. Even though there are areas for improvement, your hard work and dedication are evident. Remember that each step forward is progress. If you need help with any part of the feedback, our tutors are available to support you. Keep pushing forward and continue your impressive journey!

If you have any questions or if any part of this review needs clarification, please don’t hesitate to reach out to me on Hub (Sebastian Lucero).

Comment thread utils/Api.js Outdated
return fetch(`${this._baseUrl}/cards`, {
headers: this._headers,
}).then((res) => {
if (res.ok) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To avoid repeating response-checking logic across multiple API requests, you should define a reusable method called _checkResponse. This method should handle checking whether the server response is successful (res.ok) and process or reject it accordingly:

_checkResponse(res) {
    if (res.ok) {
        return res.json();
    }
    return Promise.reject(`Error: ${res.status}`);
}

Once defined, you can simplify your API calls by using:

.then(this._checkResponse)

Note: Make sure to pass a reference to the method (i.e., without parentheses), not the result of calling it. This ensures that the function is only executed when the .then() block is triggered.

Comment thread utils/Api.js
}

getUserInfo() {
return fetch(`${this._baseUrl}/users/me`, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

COULD BE IMPROVED

To avoid repeating the same fetch-and-check pattern in every request, consider creating a dedicated method that handles both fetching and response checking:

_request(url, options) {
  return fetch(url, options).then(this._checkResponse);
}

You can then replace direct fetch calls with this._request(...) throughout your class. This keeps your code cleaner and easier to maintain.

Comment thread src/pages/index.js
.then((data) => {
profileAvatar.src = data.avatar;
closeModal(avatarModal);
avatarForm.reset();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To prevent empty data from being submitted, you need to disable the submit button immediately after the form is reset.

Comment thread src/pages/index.js Outdated
});

avatarModalBtn.addEventListener("click", () => {
resetValidation(avatarForm, validationConfig);

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 resetValidation method should be called only when the Edit Profile form is opened, since this form is repopulated with valid data every time. This ensures that any outdated validation messages are cleared.

In contrast, the Edit Avatar form is not populated, so its input state (including validation messages) should be preserved between openings to avoid clearing validation messages unnecessarily.

Please remove this line.

Comment thread src/pages/index.js Outdated

newPostBtn.addEventListener("click", function () {
resetValidation(newPostForm, settings);
resetValidation(newPostForm, validationConfig);

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 resetValidation method should be called only when the Edit Profile form is opened, since this form is repopulated with valid data every time. This ensures that any outdated validation messages are cleared.

In contrast, the New Post form is not populated, so its input state (including validation messages) should be preserved between openings to avoid clearing validation messages unnecessarily.

Please remove this line.

Comment thread src/pages/index.js Outdated
})
.catch(console.error)
.finally(() => {
disableButton(newPostSubmitBtn, validationConfig);

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 submit button should only be disabled after receiving a successful response from the server. This ensures that if an error occurs during the request, the user can try submitting the form again.

Please remove this line.

Comment thread src/pages/index.js
openModal(avatarModal);
});

avatarModalCloseBtn.addEventListener("click", function () {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

COULD BE IMPROVED

You can set up a universal handler for all close buttons in modals. This allows you to manage all modal close functionality with a single, reusable code block.

// Select all close buttons
const closeButtons = document.querySelectorAll('.modal__close-btn');

closeButtons.forEach((button) => {
  // Find the closest modal for each button, only once
  const modal = button.closest('.modal');

  // Attach a click event listener to each close button
  button.addEventListener('click', () => closeModal(modal));
}); 

By using consistent CSS classes (like .modal__close-btn for close buttons), you ensure that every modal button behaves the same way. So, no matter how many modals you have, each close button will automatically work as expected without any additional setup.

@sebasLTripleTen sebasLTripleTen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great job, your project has been accepted!

Good luck with the next sprint, you’re doing awesome! 🚀

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