Skip to content
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

month-12/step-1_first submition #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MarinaTaras
Copy link

No description provided.

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Здравствуйте. (Нужно развернуть общий комментарий ↓)

Посмотрите на гитхабе все комментарии к коду (нужно прокрутить вниз страницу там)

Работа проделана огромная:

  • Отлично типизировали проект
  • Хорошо сделаны классы
  • Хорошо сделали визуализацию

но есть некоторые недочеты:

  • Файл app.test.js нужно удалить, так как он не относится к проекту
  • app.css нужно сделать модулем app.module.css, а в компоненте использовать модульные классы, а не строки
  • Скрин https://disk.yandex.ru/i/c8HlsB5JTPvH-A По заданию все кнопки сабмита должны быть неактивными, если в инпуте ничего нет или в нем невалидные данные.
  • Скрин https://disk.yandex.ru/i/jVb-l7YwJ0fXLg При невалидном индексе кнопки удаления/добавления по индексу активны. Нужно определять максимально возможный индекс по длине массива минус 1 и не давать указать индекс меньше нуля
  • Все события обработчиков нужно правильно типизировать. Это легко сделать. Для onChange есть тип ChangeEvent<HTMLInputElement>, а для onSubmit есть FormEvent<HTMLFormElement> в библиотеке react. Для обычных функций есть тип SyntheticEvent для их событий. Тип Response для ответа от сервера, тип KeyboardEvent для событий клавиатуры, тип RouteProps для рутов

Исправьте, пожалуйста, недочеты и работа будет принята. Пожалуйста, проверьте работоспособность проекта и наличие возможных ошибок в консоли браузера (кнопка F12) перед отправкой на ревью.

Напоминаю, что работа может быть принята только после исправления всех критических замечаний Нужно исправить.

Удачного рефакторинга кода.

const [stringData, setStringData] =
useState({ string: '', changing: [] as number[] })

function onInputChange(e: FormEvent<HTMLInputElement>) {

Choose a reason for hiding this comment

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

Можно лучше

Можно сделать универсальный кастомный хук для контроля любого количества инпутов в любых формах:

export function useForm(inputValues={}) {
  const [values, setValues] = useState(inputValues);

  const handleChange = (event) => {
    const {value, name} = event.target;
    setValues({...values, [name]: value});
  };
  return {values, handleChange, setValues};
}

Этот код помещают в отдельный файл useForm.js в папке hooks и импортируют функцию туда, где нужно контролировать инпуты

И Вам не нужно будет теперь вручную создавать функции обработки инпутов и т д. Все будет в одной строчке кода:

  const {values, handleChange, setValues} = useForm({});

const [stringData, setStringData] =
useState({ string: '', changing: [] as number[] })

function onInputChange(e: FormEvent<HTMLInputElement>) {

Choose a reason for hiding this comment

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

Все события обработчиков нужно правильно типизировать. Это легко сделать. Для onChange есть тип ChangeEvent<HTMLInputElement>, а для onSubmit есть FormEvent<HTMLFormElement> в библиотеке react. Для обычных функций есть тип SyntheticEvent для их событий. Тип Response для ответа от сервера, тип KeyboardEvent для событий клавиатуры, тип RouteProps для рутов

Есть небольшая хитрость: можно всегда проверить, какой тип у функции или события. Для этого нужно навести мышкой на атрибут onChange, onSubmit, onClick и тд. Там высветится типизация того, что принимают эти атрибуты.


function onInputChange(e: FormEvent<HTMLInputElement>) {
e.preventDefault();
setValue({ ...form, string: (e.target as HTMLInputElement).value });

Choose a reason for hiding this comment

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

(e.target as HTMLInputElement)

Это не нужно будет, когда типизируете правильно событие через ChangeEvent

import { bubbleSort, randomArr, selectionSort } from "./sorting";
import { ElementStates } from "../../types/element-states";

export type randomArrayType = {

Choose a reason for hiding this comment

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

Кастомные типы typescript принято называть в PascalCaseбольшой буквы)

state: ElementStates
}

type propsType = {

Choose a reason for hiding this comment

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

Кастомные типы typescript принято называть в PascalCaseбольшой буквы)

Copy link

@gennady-bars gennady-bars left a comment

Choose a reason for hiding this comment

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

Поздравляю! Ваша работа принята.

Вы отлично потрудились.

Удачного дальнейшего обучения.

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