add files to create first PR into master branch#29
add files to create first PR into master branch#29ivanauchynnikau wants to merge 17 commits intomasterfrom
Conversation
| import {connect} from "react-redux"; | ||
| import UserProvider from "../../providers/user"; | ||
| import {push} from "react-router-redux"; | ||
| import {LOCAL_STORAGE_KEYS} from "../../utils/js/config"; |
There was a problem hiding this comment.
может сделать папку с константами?
There was a problem hiding this comment.
Не совсем понимаю, сейчас есть файл с константами, нужно вынести его в отдельную папку?
| className="account-page__edit-button" | ||
| onClick={this.changeMode} | ||
| > | ||
| <svg width="36" height="36" viewBox="0 0 36 36" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M34.8538 1.14613C36.1662 2.45854 36.1641 4.58582 34.8538 5.89823L33.2701 7.48191L28.517 2.7298L30.1006 1.14613C31.413 -0.165175 33.5413 -0.166291 34.8538 1.14613ZM11.0966 20.1524L9.51401 26.487L15.8487 24.9034L31.6864 9.06559L26.9333 4.31348L11.0966 20.1524ZM26.8786 17.0419V31.5202H4.47977V9.12135H18.9591L23.4378 4.64158H0V35.9999H31.3583V12.561L26.8786 17.0419Z" fill="#1BA098"/></svg> |
There was a problem hiding this comment.
хорошо бы иконки держать в отдельно папке
| <div className="account-page__content"> | ||
| <div className="form-item"> | ||
| <label className="form-item__label" htmlFor="user-email">Email:</label> | ||
| <input className="form-item__input" |
There was a problem hiding this comment.
конструкцию <div><label>Title</label><input/></div> можно вынести в отдельную компоненту и переиспользовать. это облегчит шаблоны
| type="text" | ||
| value={newFirstName} | ||
| name="newFirstName" | ||
| id="first-name" |
There was a problem hiding this comment.
если делать по красоте, то у инпута должен быть айди, а у лейбла ссылка на этот айди
| import Loader from "../loader/loader"; | ||
|
|
||
|
|
||
| class InitializingContainer extends Component { |
There was a problem hiding this comment.
Как было бы лучше его переназвать?)
| <div className="order-page__product-list"> | ||
| {order.items.map(item => ( | ||
| <div className="order-page__product-item" | ||
| key={uuid()} |
There was a problem hiding this comment.
почему ты так любишь разносить это на несколько строк?
когда в одну - это более читабельно. на мой взгляд
There was a problem hiding this comment.
когда в несколько - это более читабельно. на мой взгляд ))
| this.setState({orderList: response.data}); | ||
| }) | ||
| .catch(() => { | ||
| this.props.redirectToHomePage(); |
There was a problem hiding this comment.
почему редирект на домашнюю страницу?
можно ведь показать ошибку
пользователь открывает страницу и его редиректит на домашнюю. за что? почему? :)
There was a problem hiding this comment.
Редирект тут для того что бы не показывать список покупок, если у пользовтеля нет доступа к данной странице, на бэке в этом запросе в том числе проверяется токен пользователя.
Добавил ошибку.
По хорошему, в будущем нужно будет с сервера возвращать ошибку и показывать ее. но пока так.
…ion if server returns error.
| except: | ||
| pass |
There was a problem hiding this comment.
usually it's better to at least log the error instead of simple skipping it: for future debug purposes 😉
| if email is None: | ||
| raise serializers.ValidationError('An email address is required to log in.') | ||
|
|
||
| if password is None: | ||
| raise serializers.ValidationError('A password is required to log in.') |
There was a problem hiding this comment.
this is not needed, since you don't have required=False as parameter in you fields
There was a problem hiding this comment.
не совсем понимаю, хотел бы обсудить
| serializer = self.serializer_class(data=request.data) | ||
| serializer.is_valid(raise_exception=True) |
There was a problem hiding this comment.
ah, actually you are already using this construction, so let's use it everywhere
3 participants
No description provided.