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

Metelev: HT6 #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Metelev: HT6 #78

wants to merge 1 commit into from

Conversation

fromanywhere
Copy link

No description provided.

@@ -10,7 +10,7 @@ export default class App extends PureComponent {
<div>
<Header />
<Switch>
<Route path="/" exact component={() => <h2>Home page</h2>} />
<Route path="/" exact component={Restaurants} />
Copy link
Owner

Choose a reason for hiding this comment

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

тут должен быть Redirect, а не Route

@@ -23,6 +24,11 @@ export default (state = initialState, action) =>
draft.loaded[restId] = true;
draft.error = null;
Object.assign(draft.entities, arrToMap(data));

data.forEach((product) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Из данных, которые у нас лежат в редьюсере ресторанов мы можем однозначно сказать какой продукт из какого ресторана. Теперь эта информация у нас есть в двух местах, тут и в ресторанах. Так делать не стоит.

import cn from 'classnames';

import styles from './tabs.module.css';
import { Link } from 'react-router-dom';

function Tabs({ match }) {
Copy link
Owner

Choose a reason for hiding this comment

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

Tabs - это просто компонент, чтобы рисовать табы. Он не должен быть привязан к меню, ресторанам или еще к чему-то другому.

<Route path="/restaurants/:restId/:activeId" component={Tabs} />

<Switch>
<Route
Copy link
Owner

Choose a reason for hiding this comment

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

Как-то все слишком переусложнено. Поему нельзя было оставить Menu и Reviews в том же формате с теме же пропами, только переместить внутрь Route. Так бы не пришлось их переписывать.

@@ -36,4 +41,8 @@ const mapDispatchToProps = (dispatch, ownProps) => ({
remove: () => dispatch(remove(ownProps.product.id)),
});

export default connect(null, mapDispatchToProps)(BasketItem);
const mapStateToProps = (state, props) => ({
restaurantIdByProduct: restaurantIdByProductSelector(state, props.product),
Copy link
Owner

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