Ksenia Myakinkaya #7
Conversation
| import { Pockemon_card } from '../pockemon_card/Pockemon_card'; | ||
| import './app_container.scss'; | ||
|
|
||
| export function App_container(props) { |
There was a problem hiding this comment.
принято все же писать названия классов в CamelCase
components/header/header.jsx
Outdated
| return ( | ||
| <header> | ||
| <Link to="/"><h1 className="text">Pokedex</h1></Link> | ||
| <Link to="/caught"><span className="text">Пойманные покемоны</span></Link> |
containers/CatchButtonContainer.jsx
Outdated
| if (caught===null) return (<></>) | ||
| else return( | ||
| <> | ||
| {caught && <Catch_button onClick={this.handleCatchButton} disable={caught} text='Пойман' />} |
There was a problem hiding this comment.
почему бы не вынести определение, какой текст должен быть у кнопки из jsx и упростить рендер до
return caught === null ? null :
<CatchButton onClick={this.handleCatchButton} disable={caught} text={buttonText} />
containers/CaughtContainer.jsx
Outdated
| console.log(error); | ||
| return ( | ||
| <> | ||
| {!loading && !error && total===0 && <Empty_component />} |
There was a problem hiding this comment.
я бы все же переписал данный рендер хотя бы на тернарниках - в таком виде надо ещё посидеть и подумать, сколько различных вариантов рендера у компонента :D
containers/CatchButtonContainer.jsx
Outdated
|
|
||
| componentDidMount(){ | ||
| const {id} = this.props; | ||
| fetch(`http://localhost:3000/caught_pokemons/?id=${id}`, { |
There was a problem hiding this comment.
я бы посоветовал вынести запрос из компонента, и сделать несколько уровней абстракции - например создать apiService, в котором будет создаваться дефолтная конфигурация http запросов, и который будет предоставлять методы ( GET, PUT, POST и т.д.). И также создать pokemonsService, который уже будет предоставлять методы getCaughtPokemon(id) и который уже будет вызывать apiService.get('http://localhost:3000/caught_pokemons', queryParams)
containers/PokemonPageContainer.jsx
Outdated
| const {name, img} = this.state; | ||
| return ( | ||
| <> | ||
| {!name &&<Loading />} |
There was a problem hiding this comment.
в таких случаях лучше юзать тернарный оператор
containers/CaughtContainer.jsx
Outdated
| return ( | ||
| <> | ||
| {!loading && !error && total===0 && <Empty_component />} | ||
| {total>0 && <App_container onScroll={this.handleScroll} pokemons={pokemons} />} |
There was a problem hiding this comment.
насколько я вижу CaughtUnmounted и MainUnmounted почти идентичны - за исключением одной строчки в рендере CaughtUnmounted - почему бы не вынести рендер этой строки в родительский компонент и не сделать один компонент PokemonsListContainer, который в двух разных случая будет просто оборачиваться в два различных hoc connect?
There was a problem hiding this comment.
Я думала, что стоит их разделить, потому что по смыслу это разные компоненты и может понадобиться изменить функционал одного независимо от другого.
scripts/getCaptureDate.js
Outdated
| }}) | ||
| .then(response => { | ||
| if (response.ok) { | ||
| return Promise.resolve(response); |
There was a problem hiding this comment.
зачем promise.resolve и promise.reject? почему бы просто не написать return response и throw new Error(response.statusText)?
scripts/getCaptureDate.js
Outdated
| } | ||
| }) | ||
| .then(response => response.json()) | ||
| .then(data => { |
There was a problem hiding this comment.
вот этот блок я бы все же не выносил из компонента - он довольно жестко связан с его внутренней логикой и так становиться не очевидно что же может изменить стейт компонента
2 participants
No description provided.