-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Please, expand the general comment ↓)
Dear Julian,
Unfortunately, I am unable to complete the review at this time because:
- When I like new cards, there are errors https://snipboard.io/MBYWja.jpg
- When I delete new cards, there are errors https://snipboard.io/61UVWZ.jpg
- When I reload the page, the deleted cards appear again
If you need any assistance in the meantime, our dedicated tutors are available to support you. Please don’t hesitate to reach out to them for guidance.
If your code is correct locally on your computer, then check if you have pushed all the changes to github
. I can see those changes only if they are in your branch
There was a problem hiding this comment.
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, Julian!
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:
- 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!✨
src/utils/Api.js
Outdated
}).then((res) => { | ||
if (res.ok) { | ||
return res.json(); | ||
} | ||
Promise.reject(`Error: ${res.status}`); |
There was a problem hiding this comment.
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.
COULD BE IMPROVED
You can make a special method (function ) for fetching and checking responses not to duplicate it in every request:
_request(url, options) {
return fetch(url, options).then(this._checkResponse)
}
And now you can safely replace fetch
with this._request
and remove repeated checking. And you don’t need to change anything else.
src/pages/index.js
Outdated
|
||
// avatar modal elements | ||
const avatarModal = document.querySelector("#edit-avatar-modal"); | ||
const avatarFormElement = avatarModal.querySelector(".modal__form"); |
There was a problem hiding this comment.
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 wasting resources on searching the forms in the DOM, you can efficiently access any form from document.forms
, using their unique id
attributes.
If a form has an id
attribute of avatar-form
for example, instead of using
const avatarForm = document.querySelector("#avatar-form");
you could instead use:
const avatarForm = document.forms["avatar-form"];
This is not required, but it's good to know about another way of finding forms in the DOM.
More info here https://developer.mozilla.org/en-US/docs/Web/API/Document/forms
api | ||
.getAppInfo() | ||
.then(([cards, user]) => { | ||
cards.forEach((card) => { | ||
renderCard(card, "append"); |
There was a problem hiding this comment.
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
src/pages/index.js
Outdated
const submitBtn = evt.submitter; | ||
setButtonText(submitBtn, true); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
name, | ||
about, | ||
}), | ||
}).then(this._checkResponses); |
There was a problem hiding this comment.
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 _request
method to avoid duplication of the ok
checking, headers
and baseUrl
_request(endpoint, options = {}) {
const finalOptions = {
// add the default headers
headers: this._headers,
// spread the other options if provided
...options,
};
// add the endpoint to the base url
const url = `${this._baseUrl}${endpoint}`;
return fetch(url, finalOptions).then(this._checkResponse);
}
// usage
getUserInfo() {
// pass only the endpoint here
return this._request("/users/me");
}
getInitialCards() {
// pass only the endpoint here
return this._request("/cards");
}
// and so on
No description provided.