-
Notifications
You must be signed in to change notification settings - Fork 1
Правда или действие #10
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
Правда или действие #10
Conversation
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
js/img-form-uploader.js
Outdated
| document.addEventListener('keydown', onEscapeKeydown); | ||
| }; | ||
|
|
||
| const onHashTagInput = () => { |
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.
Функции делятся на две категории:
- действие - выполняет работу с данными
- предикат (утверждение) - отвечает
true/false
Предикаты всегда именуем в форме вопроса - начинается, обычно со слов is или has
Все остальные функции всегда именуем начиная с глагола, который отвечает на вопрос "что сделать?": получить, управлять, добавить, построить, и т.д.
Именование начинающееся с on используется, но обычно в библиотеках и лучше его не применять в проекте. Примеры замен:
handleTagChange
handlePhotoSelect
| }; | ||
|
|
||
| const onHashTagInput = () => { | ||
| isHashtagsValid(inputHashtag.value); |
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.
hash и tag - это два слова или одно? Если два, то Tag с большой буквы. Если одно, то надо поправить в других функциях именование
js/util.js
Outdated
| export {getRandomNumber, getRandomItem}; | ||
| const isEscapeKey = (evt) => evt.key === 'Escape'; | ||
|
|
||
| const numDecline = (num, nominative, genetiveSingular, genetivePlural) => { |
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.
Что делает функция? Ожидается глагол основным словом
И опечатка в названии - правильно genitive
js/render-photo.js
Outdated
| @@ -1,5 +1,6 @@ | |||
| import {photos} from './create-desc.js'; | |||
| import { openBigPicture } from './render-big-photo.js'; | |||
| //import { onPhotoSelect } from './img-form-uploader.js'; | |||
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.
Если строка не нужна, не включаем ее в репозиторий
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
js/img-form-uploader.js
Outdated
| @@ -0,0 +1,49 @@ | |||
| import {isEscapeKey} from './util.js'; | |||
| import {error, isHashtagsValid} from './hashtag-validator.js'; | |||
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.
по ТЗ ожидалась валидация тегов и комментария - п.5. задания
js/hashtag-validator.js
Outdated
|
|
||
| let errorMessage = ''; | ||
|
|
||
| export const error = () => errorMessage; |
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.
именование - ожидается глагол
js/hashtag-validator.js
Outdated
| const MAX_HASHTAGS = 5; | ||
| const MAX_SYMBOLS = 20; | ||
|
|
||
| let errorMessage = ''; |
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.
удобнее реализовать объект с валидируемыми полями: тег и комментарий. Один общий модуль для валидации обоих полей.
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
| MAX_SYMBOLS_COMMENT: 140 | ||
| }; | ||
|
|
||
| let getErrorMessage = ''; |
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.
Удобнее пользоваться объектами
const errors = {
hashtags: '',
comment: '',
}
|
|
||
| let getErrorMessage = ''; | ||
|
|
||
| export const getError = () => getErrorMessage; |
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.
const getError = (field) => errors[field]
| }, | ||
| { | ||
| check:inputArray.some((item) => item.length > settings.MAX_SYMBOLS_HASHTAG), | ||
| error:`Максимальная длина одного хештега ${settings.MAX_SYMBOLS_HASHTAG} символов, включая решетку` |
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.
Идеально - вынести все тексты в константы. Не срочно, но полезно реализовать
| export {getRandomNumber, getRandomItem}; | ||
| const isEscapeKey = (evt) => evt.key === 'Escape'; | ||
|
|
||
| const getCorrectFormWord = (num, nominative, genitiveSingular, genitivePlural) => { |
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.
Если я правильно понимаю, функции мы передаем варианты слов и значение. Функция выбирает слово в соответствии со значением.
Если так, то "выбрать" в качестве глагола. Обычно это связываем с "окончанием" и можем уточнить, что является основой выбора. Варианты:
selectPluralselectPluralByCountselectWordByCount
| return true; | ||
| } | ||
|
|
||
| const rule = { |
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.
Избыточно для этого валидатора. Просто условие сделать и если нет, сохранить текст ошибки
|
|
||
| const rules = [ | ||
| { | ||
| check:inputArray.some((item) => item === '#'), |
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.
Было бы хорошим вариантом все check реализовать в виде функций и запускать их при проверке. Это исключит запуск всех кейсов и исключений
| return true; | ||
| } | ||
|
|
||
| const inputArray = inputText.split(/\s+/); |
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.
не используем слово array и object в именовании
"данные", "разделенный инпут", "теги" др.
В данном случае можно, как конкретизировать название (все же это теги), можно обобщить
🎓 Правда или действие
💥 https://htmlacademy-javascript.github.io/2607777-kekstagram-2/10/