-
Notifications
You must be signed in to change notification settings - Fork 1
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
#340-sidebar #398
#340-sidebar #398
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.
Отличная работа, парочка предложений по улучшению кода
const { slug } = useParams() | ||
const getMainCategories = useSelector(getCategorySelector) | ||
|
||
const [itemActive, setItemActive] = useState<MainCategoryInfo | BranchesData>() |
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.
Давай эти два стейта и useEffect вынесем в кастомный хук, откуда будем получать эти параметры.
Положить его можно в папку с этим же компонентом в models
branch={branch} | ||
itemName={item.name}> | ||
<ul role="list"> | ||
{item.branches.map(el => ( |
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.
Может добавим проверку, что в item есть branches?
src/features/SideBar/ui/SideBar.tsx
Outdated
@@ -20,11 +26,36 @@ export interface ISideBar { | |||
* @param {function} onClick - функция выхода из профиля handleLogOut; | |||
* @param {function} onKeyUp - функция выхода из профиля handleLogOut при нажатии клавиши Enter; | |||
* @param {JSX.Element} children - контент; | |||
* Далее, для сайдбара на странице категори; | |||
* @param {string} to - если есть ссылка для 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.
Эти параметры я бы передавал объектом, так же еще я бы добавил флаг - раскрывающийся или нет это sidebar, чтобы было сразу понятно, как мы его используем
Вынесла лишнее в хук, добавила проверку на наличие веток, перенесла все, что связано с сайдбаром категории в объект. А вот про флаг не поняла. У SideBar есть isVisible, как я поняла, как раз для того, чтобы выделить раскрывающийся список и нераскрывающийся. Или нужен какой-то другой флаг? |
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.
Отлично, только одно маленькое замечание
src/features/SideBar/ui/SideBar.tsx
Outdated
|
||
import styles from './SideBar.module.scss' | ||
|
||
export interface ISideBar { | ||
type TCategorySidebar = { |
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.
этот тип выносим в папку model, в папке с компонентом у нас только типизация компонента
Сделала сайдбар, выглядит вот так:
Активная ссылка подсвечивается, если она из подкатегорий, главная категория сразу открыта при переходе, чтобы отделить ее от остальных.