-
Notifications
You must be signed in to change notification settings - Fork 0
React: Routing and Hooks #3
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
base: unit-testing
Are you sure you want to change the base?
Conversation
kosmodromm
left a comment
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.
lgtm
| onKeyDown={ | ||
| isClickable | ||
| ? (e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| onClick?.(id); | ||
| } | ||
| } | ||
| : undefined | ||
| } |
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.
вынеси в handleKeyDown()
| export class Card extends Component<CardProps> { | ||
| getValue(key: string, value: string): string { | ||
| export const Card = ({ character, onClick, variant = 'list' }: CardProps) => { | ||
| const { id, name, status, species, gender, image, origin, location } = |
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.
лучше сделать enum в character.ts для status, gender и variant. типа такого enum CharacterGender {
FEMALE = 'Female',
MALE = 'Male',
GENDERLESS = 'Genderless',
UNKNOWN = 'unknown',
}
| type CardProps = { | ||
| character: Character; | ||
| onClick?: (id: number) => void; | ||
| variant?: 'list' | 'details'; |
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.
будет прозрачнее, если variant будет обязательный
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.
да. конечно
| element: <Navigate to={PATHS.CHARACTER} replace />, | ||
| }, | ||
| { | ||
| path: '/character', |
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.
почему не используются пути из paths.ts?
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.
пропустил, когда рефакторил
| path: PATHS.NOT_FOUND, | ||
| element: <ErrorLayout />, | ||
| errorElement: <FallBack />, | ||
| children: [{ path: PATHS.NOT_FOUND, element: <NotFoundPage /> }], |
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 const useCharactersQuery = (query: string, page: number) => | ||
| useQuery({ | ||
| queryKey: ['characters', { query, page }], |
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.
вынести в QUERY_KEYS объект
| const get = (): T => { | ||
| try { | ||
| const item = localStorage.getItem(key); | ||
| if (item === null) return defaultValue; | ||
|
|
||
| if (typeof defaultValue === 'string') { | ||
| return item as T; | ||
| } | ||
| return JSON.parse(item) as T; | ||
| } catch { | ||
| return defaultValue; | ||
| } | ||
| }; | ||
|
|
||
| const [value, setValue] = useState<T>(get); |
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.
сейчас get вызывается при каждом ререндере, нужно ли это?
можно поменять на const [value, setValue] = useState(() => get()); и тогда выхов get будет только при маунте
| export function useLocalStorage<T>(key: string, defaultValue: T) { | ||
| const get = (): T => { | ||
| try { | ||
| const item = localStorage.getItem(key); | ||
| if (item === null) return defaultValue; | ||
|
|
||
| if (typeof defaultValue === 'string') { | ||
| return item as T; | ||
| } | ||
| return JSON.parse(item) as T; | ||
| } catch { | ||
| return defaultValue; | ||
| } | ||
| }; | ||
|
|
||
| const [value, setValue] = useState<T>(get); | ||
|
|
||
| const set = useCallback( | ||
| (value: T) => { | ||
| try { | ||
| if (typeof value === 'string') { | ||
| localStorage.setItem(key, value); | ||
| } else { | ||
| localStorage.setItem(key, JSON.stringify(value)); | ||
| } | ||
| setValue(value); | ||
| } catch {} | ||
| }, | ||
| [key], | ||
| ); |
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.
лучше всегда сериализовать/десериализовать JSON.stringify и JSON.parse. попробуй переделай. код станет проще и будет меньше потенциальных ошибок
| const searchQueryFromURL = searchParams.get('name') || ''; | ||
| const pageFromURL = Number(searchParams.get('page') || 1); |
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.
почитай про ?? оператор. тут нужен он
| componentDidMount(): void { | ||
| this.fetchCharacters(this.state.searchQuery); | ||
| } | ||
| const searchQuery = searchQueryFromURL || ''; |
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.
зачем? это уже делается в searchQueryFromURL
Uh oh!
There was an error while loading. Please reload this page.