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

Project 9 - First Submission #1

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

Project 9 - First Submission #1

wants to merge 37 commits into from

Conversation

kn8-codes
Copy link
Owner

No description provided.

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Dear Natahniel, (Please, expand the general comment ↓)

Unfortunately I have to reject the project:

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Dear Natahniel, (Please, expand the general comment ↓)

Please, scroll down the page on github to see my remarks right in the code

Good job but there are some remarks:

  • validation.js is not needed anymore
  • The avatar popup doesn't close after successful changing of the avatar
  • When I remove 3 cards in a row there are cought errors https://disk.yandex.ru/i/QwLMnfWHFMx3vg
  • According to the task you need to change submit buttons text (Saving…) while waiting for the server response. And the user should see it.
  • See here https://disk.yandex.ru/i/HRydRGRU1Zsu7Q popupWithConfirmation.js should be in UpperCase like the other files with classes
  • According to the task you need to pass full headers into the constructor of Api. See here https://disk.yandex.ru/i/J93dbJp7eypXKQ . And in the code you need to use only: headers: this._headers (not to duplicate the code)
  • Please, delete removeEventsListeners because you don't have such a method in Popup (and you don't need it)
  • Please, add in PopupWithForm.js method renderLoading like you have in PopupWithConfirmation. It's a very nice method for text changing
  • Please, delete the low dash from methods if you want to use them outside a class because the low dash shows a private method which is used only inside the class.
  • You need to close popups only in blocks then after a successful response from the server so that when there is an error it would be opened (the inputs will not be cleared and the user could send the data again to the server) and if you change the button text you could see the change as well

Please, correct everything and your project will be accepted.

Good luck with the refactoring

@@ -1,4 +1,4 @@
# Project 8: Around The U.S.
# Project 9: Around The U.S.

Choose a reason for hiding this comment

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

According to the task you need to change submit buttons text (Saving…) while waiting for the server response. And the user should see it.

getUserInfo() {
return fetch(this._url + "users/me", {
headers: this._headers,
}).then(this._getResponse);

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 make a special method for fetching and checking responses not to duplicate it in every request:

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

And now you can safely replace fetch with this._request and remove repeated checking. And you don’t need to change anything else.

src/components/Api.js Outdated Show resolved Hide resolved
src/components/Api.js Outdated Show resolved Hide resolved
src/components/Card.js Outdated Show resolved Hide resolved
function handleProfileEditSubmit(data) {
userInfo.setUserInfo(data.name, data.profession)
editProfilePopup.close()
api.updateProfile(data.name, data.profession)

Choose a reason for hiding this comment

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

Could be improved

If it’s interesting for you here is how we can make a universal function for handling any submit. We can get rid of such duplicating as loading effect, closing and catching errors

// You can make a universal function that accepts a request, popup instance and optional loading text  
function handleSubmit(request, popupInstance, loadingText = "Saving...") {

  // here we change the button text
  popupInstance.renderLoading(true, loadingText);
  request()
    .then(() => {
      // We need to close only in `then`
      popupInstance.close()
    })
      // we need to catch possible errors
      // console.error is used to handle errors if you don’t have any other ways for that
     .catch(console.error)
    // in `finally` we need to return the initial button text back in any case
    .finally(() => {
      popupInstance.renderLoading(false);
    });
}

You need to pass the request into the call of handleSubmit like that:

// here is an example of the profile form handling
function handleProfileFormSubmit(inputValues) {
  // we create a function that returns a promise
  function makeRequest() {
    // `return` lets us use a promise chain `then, catch, finally` inside `handleSubmit`
    return api.editProfile(inputValues).then((userData) => {
      userInfo.setUserInfo(userData)
    });
  }
  // Here we call the function passing the request, popup instance and if we need some other loading text we can pass it as the 3rd argument
  handleSubmit(makeRequest, profilePopup);
}

src/pages/index.js Outdated Show resolved Hide resolved
src/pages/index.js Show resolved Hide resolved
src/pages/index.js Outdated Show resolved Hide resolved
src/pages/index.js Outdated Show resolved Hide resolved
Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

The code is much better now but:

}

removeEventsListeners() {
super.removeEventsListeners();
removeEventListeners() {

Choose a reason for hiding this comment

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

removeEventListeners() is not needed in the project. You change the function in line 25 and that's enough

@@ -6,6 +6,7 @@ export default class PopupWithForm extends Popup {
this.handleFormSubmit = handleFormSubmit;
this._popupForm = this._popupElement.querySelector(".modal__form");
this._inputList = this._popupForm.querySelectorAll(".modal__input");
this._submitButton = this._popupElement.querySelector("#avatar-save")

Choose a reason for hiding this comment

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

this._submitButton = this._popupElement.querySelector("#avatar-save")

Please, delete it

The button should be found correctly (universally). It's not only for the avatar

src/pages/index.js Outdated Show resolved Hide resolved
editProfilePopup.close();
}
.then((data) => {
avatarPopup.renderLoading(false)

Choose a reason for hiding this comment

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

You need to return the default button text only in blocks finally so that the button could get the text back in any case.

userInfo.setUserInfo(data)
})
.then(avatarPopup.close())
.finally(avatarPopup.renderLoading(true))

Choose a reason for hiding this comment

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

In blocks then, catch, finally you need to pass only the reference to the function (not a function call) otherwise the function is called immediately without waiting for the response from the server. You can pass an unnamed arrow function as well.

avatarPopup.renderLoading(false)
userInfo.setUserInfo(data)
})
.then(avatarPopup.close())

Choose a reason for hiding this comment

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

In blocks then, catch, finally you need to pass only the reference to the function (not a function call) otherwise the function is called immediately without waiting for the response from the server. You can pass an unnamed arrow function as well.

Please, correct it everywhere

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Dear Natahniel, (Please, expand the general comment ↓)

Unfortunately I have to reject the project:

You haven't corrected my remarks:

  • See here https://disk.yandex.ru/i/HRydRGRU1Zsu7Q popupWithConfirmation.js should be in UpperCase like the other files with classes
  • According to the task you need to change submit buttons text (Saving…) while waiting for the server response. And the user should see it.
  • And maybe others (see them in the prevous iteration)

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Dear Natahniel, (Please, expand the general comment ↓)

Unfortunately I have to reject the project because nothing has been changed except for PopupWithConfirmation.js. Please, check all my remarks from the 2 previous iterations. See here #1 (comment) and here #1 (comment) and others

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

One more step is needed:

  • this._submitButtonText = "Delete"; should be deleted. You have the correct this._submitButtonText in line 8 with text Yes (it's fixed in the constructor authomatically)
  • According to the task you need to change submit buttons text (Saving…) while waiting for the server response. And the user should see it.
  • You haven't corrected my remark https://github.com/kn8-codes/se_project_aroundtheus/pull/1/files/31becbe36b838625f6d8b7411bc5c3eaae35b01e..686a708f67887fd3f368a4618a0c2e9d90731066#r1354377598
  • In blocks then, catch, finally you need to pass only the reference to the function (not a function call) otherwise the function is called immediately without waiting for the response from the server. You can pass an unnamed arrow function as well.
  • You need to return the default button text only in blocks finally so that the button could get the text back in any case.
  • addCardPopup.renderLoading(true); should be in line 165 (before the request) to start loading effect
  • You don't have renderLoading for the profile submit button. You should add it into handleProfileEditSubmit

@@ -12,7 +12,7 @@ renderLoading(isLoading, loadingText = "Deleting...") {
if (isLoading) {
this._submitButton.textContent = loadingText;
} else {
this._submitButtonText = "Save";
this._submitButtonText = "Delete";

Choose a reason for hiding this comment

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

this._submitButtonText = "Delete"; should be deleted. You have the correct this._submitButtonText in line 8 with text Yes (it's fixed in the constructor authomatically)

@@ -153,7 +153,7 @@ function handleAvatarChange(data) {
avatarPopup.renderLoading(false)
userInfo.setUserInfo(data)
})
.then(avatarPopup.close())
.then(avatarPopup.close)
.finally(avatarPopup.renderLoading(true))
.catch((err) => {
console.log(console.error);

Choose a reason for hiding this comment

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

function handleProfileEditSubmit(data) {
  api.updateProfile(data.name, data.profession)
    .then((data) => {
      userInfo.setUserInfo(data)
    })
    .catch((err) => {
      console.log(console.error);
    });
  editProfilePopup.close();
}

You don't have renderLoading for the profile submit button

editProfilePopup.close(); should be in then

    .then((data) => {
      userInfo.setUserInfo(data);
      editProfilePopup.close();  // here
    })

@@ -153,7 +153,7 @@ function handleAvatarChange(data) {
avatarPopup.renderLoading(false)
userInfo.setUserInfo(data)
})
.then(avatarPopup.close())
.then(avatarPopup.close)
.finally(avatarPopup.renderLoading(true))

Choose a reason for hiding this comment

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

.finally(avatarPopup.renderLoading(true))

This is not correct. See my remark https://github.com/kn8-codes/se_project_aroundtheus/pull/1/files/31becbe36b838625f6d8b7411bc5c3eaae35b01e..686a708f67887fd3f368a4618a0c2e9d90731066#r1354377598

This is the way it should work (with false) - you stop the loading in finally rather than start it

    .finally(() => avatarPopup.renderLoading(false))

@@ -153,7 +153,7 @@ function handleAvatarChange(data) {
avatarPopup.renderLoading(false)

Choose a reason for hiding this comment

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

avatarPopup.renderLoading(false) should be in finally

You need to return the default button text only in blocks finally so that the button could get the text back in any case.

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Dear Natahniel, (Please, expand the general comment ↓)

Unfortunately I have to reject the project:

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

One more step is still needed:

  • avatarPopup.renderLoading(true) should be in line 154 before the request to change the button text
  • console.log(console.error); should be console.error(err);. Please, pay attantion the console.error is a method for logging errors
  • .then(addCardPopup.close()) should be .then(addCardPopup.close)
  • In blocks then, catch, finally you need to pass only the reference to the function (not a function call) otherwise the function is called immediately without waiting for the response from the server. You can pass an unnamed arrow function as well.

@@ -156,23 +157,22 @@ function handleAvatarChange(data) {
userInfo.setUserInfo(data)

Choose a reason for hiding this comment

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

avatarPopup.renderLoading(true) should be in line 154 before the request to change the button text

function handleAvatarChange(data) {
  avatarPopup.renderLoading(true)  // here
  api.updateAvatar(data)
    .then((data) => {

Comment on lines 147 to 148
.catch((err) => {
console.log(console.error);

Choose a reason for hiding this comment

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

console.log(console.error); should be console.error(err);. Please, pay attantion the console.error is a method for logging errors

@@ -156,23 +157,22 @@ function handleAvatarChange(data) {
userInfo.setUserInfo(data)
})
.then(avatarPopup.close)
.finally(avatarPopup.renderLoading(false))
.finally(() => avatarPopup.renderLoading(false))
.catch((err) => {
console.log(console.error);

Choose a reason for hiding this comment

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

console.log(console.error); should be console.error(err);. Please, pay attantion the console.error is a method for logging errors

.then(addCardPopup.close)
.finally(() => {
addCardPopup.renderLoading(false)})
.then(addCardPopup.close())

Choose a reason for hiding this comment

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

.then(addCardPopup.close()) should be .then(addCardPopup.close)

In blocks then, catch, finally you need to pass only the reference to the function (not a function call) otherwise the function is called immediately without waiting for the response from the server. You can pass an unnamed arrow function as well.

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Everything is almost fine but:

@@ -170,8 +170,8 @@ function handleAddCardSubmit(data) {
.then((res) => {
const card = createCard(res.data);

Choose a reason for hiding this comment

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

createCard(res.data);

You need to use only res (the api has been fixed recently)

    .then((res) => {
      const card = createCard(res);

Choose a reason for hiding this comment

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

createCard(res.data);

You need to use only res (the api has been fixed recently)

    .then((res) => {
      const card = createCard(res);

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Dear Natahniel, (Please, expand the general comment ↓)

Unfortunately I have to reject the project:


createCard(res.data);

You need to use only res (the api has been fixed recently)

    .then((res) => {
      const card = createCard(res);

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