Skip to content

Project 9 - #3

Open
codelur wants to merge 6 commits into
mainfrom
project-9
Open

Project 9#3
codelur wants to merge 6 commits into
mainfrom
project-9

Conversation

@codelur

@codelur codelur commented Nov 13, 2024

Copy link
Copy Markdown
Owner

No description provided.

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

↓Expand to see the whole summary↓

Hello, Salvador!

Great job on submitting your project 9! This time you’ve implemented Api integration. So you can create your own simple sites from now on!

Parts I really liked in your project:

  • Good job that you close modals only in blocks then
  • Good job that you catch possible errors at the end of server requests.
  • Good job that you return the button text in finally
  • Good job that you get the whole info with Promise.all

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

Needs correcting:

  • All comments in the code and the comments below:
  • Please, remove the white square from the hearts https://snipboard.io/6OmEKC.jpg
  • Please, make the confirmation modal look better on 320px (mobile)
  • The avatar modal styles should match the design (including the font). All your modals have the same styles because they need to use the same css class names for similar elements like close icons, inputs, save buttons
  • The code of ok checking is repeated in every request of the project. To avoid it you need to make a special method _checkResponse that will check any response from the server if it’s ok or not.

There are some comments in your project that can help you make it even better. Please check the Could be improved comments.

A reminder: It’s necessary to fix all comments to get the project accepted. And Could be improved comments are optional, they will be up to you if you want to implement them.

Your hard work on this project is truly commendable. While there are still some adjustments needed. If you need any assistance with the feedback or understanding certain concepts, our tutors are here to help. Keep moving forward and maintain your dedication — you are doing an amazing job!✨

Comment thread src/utils/Api.js Outdated
Comment on lines +17 to +22
}).then((res) => {
if (res.ok) {
return res.json();
}
return Promise.reject(`Error: ${res.status}`);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

.then(res => {
                if (res.ok) {
                    return res.json();
                }
                return Promise.reject(`Error ${res.status}`);
            })

The code of ok checking is repeated in every request of the project. To avoid it you need to make a special method _checkResponse that will check any response from the server if it’s ok or not.

_checkResponse(res) {
        // here is the code of the checking
    }

And now you can replace redundant repeating with the only line:

 .then(this._checkResponse)

Please, pay attention that you need to pass only the reference for the method rather than the call.

Comment thread src/pages/index.js
Comment on lines +27 to +29
api
.getAppInfo()
.then(([cards, userInfo]) => {

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 that you get the whole info with Promise.all

Comment thread src/pages/index.js Outdated
profileNameElement.textContent = nameInput.value;
profileJobElement.textContent = jobInput.value;
closeModal(editProfileModal);
setButtonText(evt.submitter, true);

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

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, resetting and catching errors

// define a function for changing the button text. It accepts 4 params (the 2 last are optional with default texts)
export function renderLoading(isLoading, button, buttonText='Save', loadingText='Saving...') {
  if (isLoading) {
    button.textContent = loadingText
  } else {
    button.textContent = buttonText
  }
}

// define a universal function that accepts a request function, event and a default loading text 
function handleSubmit(request, evt, loadingText = 'Saving...') {
 // You need to prevent the default action in any submit handler
  evt.preventDefault();

  // the button is always available inside `event` as `submitter`
  const submitButton = evt.submitter;
  // fix the initial button text
  const initialText = submitButton.textContent;
  // change the button text before requesting
  renderLoading(true, submitButton, initialText, loadingText);
  // call the request function to be able to use the promise chain
  request()
    .then(() => {
      // any form should be reset after a successful response 
      // evt.target is the form in any submit handler		
      evt.target.reset();
    })
      // 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)      

    // and in finally we need to stop loading
    .finally(() => {
      renderLoading(false, submitButton, initialText);
    });
}

Here is an example of handling the profile form submit:

function handleProfileFormSubmit(evt) {
  // create a request function that returns a promise
  function makeRequest() {
    // `return` lets us use a promise chain `then, catch, finally`
    return editProfile(nameInput.value, jobInput.value).then((userData) => {
      userName.textContent = userData.name;
      userJob.textContent = userData.about;
    });
  }
  // here we call handleSubmit passing the request and event (if you want a different loading text then you need to pass the 3rd param)
  handleSubmit(makeRequest, evt);
}

So, this way you can remove a lot of code duplicating. You will not need to search buttons, pass initial button texts and so on.

handleSubmit and renderLoading should be placed in utils.js, because they are utility functions

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

closepreviewImageButton.addEventListener("click", () => {

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 make a universal handler for any close buttons.

It will look something like this:

// Find all close buttons
const closeButtons = document.querySelectorAll('.modal__close');

closeButtons.forEach((button) => {
  // Find the closest popup only once
  const popup = button.closest('.modal');
  // Set the listener
  button.addEventListener('click', () => closePopup(popup));
});

That’s why we make universal css classes for similar elements: here it’s modal__close for any close button.

So now you can add 100 modals: their close buttons will be handled automatically.

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

You’ve done a great job!

Your project has been accepted.

Good luck!

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