Pull request to make a code review #1
Conversation
| @@ -0,0 +1,37 @@ | |||
| import React from 'react'; | |||
| import { Route, Routes } from 'react-router-dom'; | |||
| import BreadcrumbsContextProvider from '../../contexts/BreadcrumbsContext'; | |||
There was a problem hiding this comment.
To get rid of these hard-to-read and write paths ( ../../someComponent ) you can use aliases (@someComponent). They can be configured in the webpack.config and tsconfig.json for typing to work correctly
| list-style: none; | ||
| } | ||
|
|
||
| .list li { |
There was a problem hiding this comment.
Not too good to use specific selectors (like li or li button) because in your jsx you can change the element to div for example, but will forget to change it here.
| const { breadcrumbs } = useContext(BreadcrumbsContext); | ||
| const navigate = useNavigate(); | ||
|
|
||
| const onBreadcrumbClick = (url: string, index: number) => { |
| <ul className={styles.list}> | ||
| {breadcrumbs.map(({ url, name }, index) => ( | ||
| <li key={index}> | ||
| <button className={index === breadcrumbs.length - 1 ? styles.breadcrumbActive : ''} onClick={() => onBreadcrumbClick(url, index)}>{name}</button> |
There was a problem hiding this comment.
For this libraries like "class names" can be used https://www.npmjs.com/package/classnames
| filterContainerRef.current = document.getElementById('filters-block'); | ||
| }, []); | ||
|
|
||
| const onChangeSelect = (value: string, name: string, type: FilterType) => { |
There was a problem hiding this comment.
line: 29, 33, 37 - not required function mapping.
| <li className={styles.filter} key={label}> | ||
| <label htmlFor={label}>{name}</label> | ||
| {type === FilterType.SELECT_ONE && ( | ||
| <FilterSelect initialValue={(intialAppliedFilters[label] || 'All') as string} onChange={(value) => onChangeSelect(value, label, FilterType.SELECT_ONE)} name={label} options={options} /> |
There was a problem hiding this comment.
line: 54, 57 "magic numbers", What are 'All' and '' and why are they the initial values? - these things need to be in constants
| import { AppliedFilter, Filter } from '../interfaces/Filter'; | ||
|
|
||
| export interface FiltersContext { | ||
| transactionFilters: Filter[]; |
There was a problem hiding this comment.
Maybe transactionFilters, and appliedTransactionFilters can be one entity?
| const { cards, cardsFilters} = useContext(DataContext); | ||
| const { appliedCardFilters, setAppliedCardFilters } = useContext(FiltersContext); | ||
| const [page, setPage] = useState(1); | ||
| const filteredCards = useMemo(() => (cards.filter((card) => { |
There was a problem hiding this comment.
Filtration logic should not be in the component but in a separate file
| } | ||
| )})), [transactions, appliedTransactionFilters]); | ||
|
|
||
| const updateFilters = (value: string | { min: number, max: number }, name: FilterName) => { |
There was a problem hiding this comment.
Logic duplication with CardsList, should be in a separate file, and imported in theses components
I finished the project but in the end I had to be faster so there are many things that I would change, or make reusable.
I'm going to shown them here in the format of a code review