-
Notifications
You must be signed in to change notification settings - Fork 15
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
order-list-item #88
order-list-item #88
Conversation
@@ -0,0 +1,15 @@ | |||
Order list item: | |||
|
|||
const { EProfileStatus } = require ('app/api/types'); |
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.
а это так работает да ?
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.
Это работает. Если здесь что-то неправильно, напиши.
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.
все правильно если работает. я прост не знал что так можно)
return ( | ||
<div className="orders-list-item"> | ||
<div className="orders-list-item__column-logo"> | ||
<div className="orders-list-item__column-logo__logo"> |
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.
тут можно везде без враперов обойтись. переделывать не надо (пока). но надо всегда помнить как мы не любим когда браузеры тормозят и потребляют много памяти ))
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.
AccountItem можно посмотреть как пример
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.
Ты имеешь в виду, что здесь лучше использовать display: grid;
?
<div className="orders-list-item__value"> | ||
{this.props.duration} hours | ||
</div> | ||
<div> </div> |
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.
а это что за штука?)
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.
Да, мне тоже не нравится эта штука. <div> </div>
в данном случае играет роль пустой третьей строки. И две первые становятся вровень со строками предыдущих колонок. Без
содержимое контейнера растягивается. Возможно этой проблемы не будет, если сделать всю разметку через display: grid;
@ordersListItem-fontFamily: Roboto, -apple-system, BlinkMacSystemFont, | ||
'Segoe UI', 'PingFang SC', 'Hiragino Sans GB', 'Microsoft YaHei', | ||
'Helvetica Neue', Helvetica, Arial, sans-serif; | ||
@ordersListItem-label-fontColor: #95989a; |
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.
эти цвета и шрифты должны быть в guide.less и надо оттуда их использовать. в том и смысл guide.less )
|
||
// External variables | ||
|
||
@ordersListItem-fontFamily: Roboto, -apple-system, BlinkMacSystemFont, |
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.
@ordersListItem-fontFamily: @font-family
|
||
Input with prefix: | ||
|
||
<div style={{display:'flex', 'flex-direction': 'row' }}> |
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.
u can just write
Input with prefix:
<Input prefix="from" />
one more:
<Input prefix="to" />
|
||
// External variables | ||
|
||
@listHeader-fontColor: #95989a; |
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.
u must take this values from guide.less
<div className={cn('list-header', this.props.className)}> | ||
<div className="list-header__sortings"> | ||
Sort by: | ||
<div className="list-header__sortings__container"> |
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 of element is not allowed in BEM notation
u must use another block here (or element)
<button | ||
key={orderKey} | ||
className={cn( | ||
'list-header__sortings__container__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.
same like previous
import { IListHeaderProps } from './types'; | ||
|
||
export class ListHeader extends React.Component<IListHeaderProps, any> { | ||
constructor(props: IListHeaderProps) { |
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.
redundant. u can just skip constructor
|
||
public render() { | ||
return ( | ||
<div className="orders"> |
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.
please use more specific block name
import * as cn from 'classnames'; | ||
|
||
export class OrdersListItem extends React.Component<IOrdersListItemProps, any> { | ||
constructor(props: IOrdersListItemProps) { |
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.
u can skip ctr definiton
import { ListHeader } from '../list-header'; | ||
import { IOrdersProps } from './types'; | ||
|
||
export class Orders extends React.Component<IOrdersProps, any> { |
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.
we must use PureComponent for complex component
|
||
const Container = observer(() => | ||
<Orders | ||
header={{ |
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.
we must use PureComponent for complex component
but this line will kill this optimisation. nested object is not good idea for props
export class Orders extends React.Component<any, any> { | ||
constructor(props: any) { | ||
super(props); | ||
this.state = initialState; |
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.
this component must not have this.state.
rootStore.orderList - is its state
…derProps. Goal: to have flat props to make Orders shouldComponentUpdate performance better.
…llet into OrderPreviewItem
No description provided.