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

Review #9

Open
wants to merge 27 commits into
base: review
Choose a base branch
from
Open

Review #9

wants to merge 27 commits into from

Conversation

Nikita-Kuzhl
Copy link
Owner

No description provided.

Copy link

@Krimsit Krimsit left a comment

Choose a reason for hiding this comment

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

Работа достаточно хорошая. Молодец, что написал апишку на ноде, знания как пишутся апишки и как они работают очень полезны в работе. По фронту могу выделить некоторые замечания:

  1. Побольше дели на компоненты, например всякие инпуты, селекты и прочие выноси в отдельные компоненты. Таки образом небе не придётся прописывать каждый раз логику для них, а только менять стили при надобности.
  2. Для большей читабельности кода отделяй логические блок. Например сначала у тебя идёт блок с объявлением хуков, затем состояния и рефы, потом функции и далее эффекты с вёрсткой.
  3. Импорты компонентов. Не обязательно для каждого компонента создавать index.ts(js), можно импортировать их так:
import Card from "Card/Card.ts(js)"

Так же в папках где много разных компонентов лучше создать входной файл index.ts(js) в котором ты будешь экспортировать все компоненты. Пример файла index.ts:

export { default as Card } from "./Card/Card.ts"
export { default as Header } from "./Header/Header.ts"

А импорт будет происходить так:

import { Card, Header } from "components"
  1. Старайся всякую логику по возможности выносить в отдельные хуки.
  2. Подчищай console.log, и закомментированный и ненужный код.
  3. Чтоб не запускать api и client отдельно, их можно объеденить в докере

import { Route, Routes } from 'react-router-dom'
import { useAppSelector } from './app/hooks'
import Authorization from './pages/Authorization'
import Catalog from './pages/Catalog'
Copy link

Choose a reason for hiding this comment

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

Совет: чтобы не засорять код в папках с множеством экспортируемых файлов можно создать файл index.js(ts) в котором у ты будешь экспортировать эти файлы:

export { default as Main } from "./Main"
export { default as Catalog} from "./Catalog"

Благодаря этому ты можешь прописать import один раз и просто внутри импорта прописывать нужные компоненты:

import { Main, Catalog } from "./pages"

Copy link
Owner Author

Choose a reason for hiding this comment

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

Спасибо за ревью!) Учту все ошибки.

import News from './pages/News'
import NewsList from './pages/NewsList'
import Registration from './pages/Registration'
import './styles/index.scss'
Copy link

Choose a reason for hiding this comment

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

Совет: для большей читабельности кода разделяй импорты. Например сначала импортируются библиотеки, затем функции/хуки, потом компоненты и в самом конце стили. Пример:

import { useState } from "react"

import { useCustomHook } from "hooks"

import { Input } from "components"

import "styles" 

await signIn(e)
}
useEffect(() => {
if (result.isSuccess && result.data.status === 200) {
Copy link

Choose a reason for hiding this comment

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

Эти действия можно перенести в useSignInMutation

placeholder='Пароль'
className={clsx(
styles.input,
errors.password ? styles.warning : '',
Copy link

Choose a reason for hiding this comment

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

Библиотека clsx поддерживает функционал добавления класса в зависимости от условий

clsx(s.class1, ..., {
  [styles.warning]: !!errors.password
})

)}
</div>
<div className={styles.input__item}>
<input
Copy link

Choose a reason for hiding this comment

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

input лучше вынести в отдельный компонент

heading: string
}

interface IParams {
Copy link

Choose a reason for hiding this comment

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

Лучше выносить интерфейсы и типы в отдельный файл

@@ -0,0 +1,22 @@
import React, { FC } from 'react'
Copy link

Choose a reason for hiding this comment

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

Не обязательно импортировать React, можно просто:

import { FC } from "react"

{checkedApartParams.map((i) => (
<button
onClick={() =>
param.checkbox.includes(i.value)
Copy link

Choose a reason for hiding this comment

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

Можно сократить до:

setParam({
  ...param,
  checkbox:  param.checkbox.includes(i.value) ? param.checkbox.filter((item) => item !== i.value) : [...param.checkbox, i.value]
})

<TelephoneIcon />
Контакты
</button>
{favorites.includes(item.id) ? (
Copy link

Choose a reason for hiding this comment

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

Можно упростить до:

<button onClick={() => handleClickFavorite()} className={styles.heart__button}>
  <p>{favorites.includes(item.id) ? "Добавлено" : "В закладки"}</p>
  <HeartIcon style={favorites.includes(item.id) ? styles.heart__icon_active : styles.heart__icon} width={15} height={15} />
</button>

@@ -0,0 +1,28 @@
import styles from './PayFooter.module.scss'
const payList = [
Copy link

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