-
Notifications
You must be signed in to change notification settings - Fork 21
Shakhov #20
base: master
Are you sure you want to change the base?
Shakhov #20
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.
Задание сделано хорошо, нужно поправить небольшие замечания что оставлены к коду.
Оригинальное решение с выбором пола.
Общие замечания:
- При открытии клавиатуры сползают кнопки
- Если нажимать на кнопку “удалить” несколько раз подряд почему-то меняются аватарки
- Пожалуйста, удали из репозитория папку .idea. Это моя изначальная ошибка. Можно забрать изменения из моего репозитория, там исправлена эта проблема
private String gender; | ||
private ImageView photoView; | ||
private int photoResId = 0; | ||
private RecyclerView recyclerView; |
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.
Не обязательно делать полем класса
} | ||
|
||
private boolean isStudentProfileValid() { | ||
if (firstName.isEmpty() || secondName.isEmpty()) return false; |
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.
Лучше всегда ставить фигурные скобки
@@ -1,9 +1,143 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<android.support.constraint.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android" | |||
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" |
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.
- Используются привязки right/left. Лучше end/start
- По возможности стоит уменьшать вложенность лейаутов
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.
Такой вопрос: стоить ли нам везде использовать start/end, если мы не ориентируемся на арабские страны, может ли верстка поехать где-то или просто будет зеркально отображена? Нужен ли start/end, если мы его никак не тестируем? Используя left/right, мы всегда уверены что верстка будет одинаковая, пусть и неудобная для каких то народностей.
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.
Лучше привыкать сразу к start/end
Тестировать это не нужно, он работает 1 в 1 как left/right но с возможностью отзеркаливания
Верстка будет одинаковая =)
android:layout_height="wrap_content" | ||
android:text="@string/delete" | ||
|
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.
Пустая строка
|
@Denispok И подтяни изменения из основного репозитория пожалуйста, что бы не было конфликтов |
Как подтянуть, чтобы ничего не сломать? |
Проще всего сделать пул реквест из моего репозитория в свой и смержить |
private EditText secondNameView; | ||
private String secondName; | ||
private Spinner genderView; | ||
private ArrayAdapter genderAdapter; |
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.
Для дженериков стоит указывать тип
Работа принята. |
No description provided.