-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: mobile menu #129
base: master
Are you sure you want to change the base?
feat: mobile menu #129
Conversation
3d1b4dd
to
dbb1723
Compare
shouldForwardProp: prop => prop !== 'active', | ||
})<HorizontalMenuItemProps>(({active, theme}) => ({ | ||
cursor: 'pointer', | ||
fontFamily: theme.font.family, |
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.
типографику через Typo
примитив или Heading
, Paragraph
или Text
компоненты
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.
У меня ссылка, надо pointer, hover задавать и т.д. Heading, Paragraph, Text - обычные компоненты, у которых нужные свойства не переопределить.
Что лучше,
-
Сделать один семантический styled компонент, не наследуясь, со всеми стилизациями в одном месте (как сейчас)? Пропсов ему задавать не надо.
-
По аналогии с Text сделать Link на теге Typo, часть стилей захреначить через пропсы, а часть через inline css? Как тут понять, что хардкодить в стилях, а что в пропсы?
-
унаследоваться от Typo, часть стилей захреначить через пропсы, часть захардкодить в styled?
Меня смущает, что в компоненте в 2, 3 стилизация делается 2мя способами: пропсы и стили, цельность хуже, лучше что-то одно.
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.
Пока оставил, до готовности LinkControl
const levels = new MenuLevelBuilder(items).levels(activeId) | ||
const level0 = levels[0] | ||
const level1 = levels[1] | ||
const level2 = levels[2] |
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.
В дизайне одно подменю. Делал 2, что б протестить, могу убрать level2. Каждый уровень может быть кастомный, рисовать в цикле не вариант.
/** | ||
* Active menu item id | ||
*/ | ||
activeId: string |
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.
я бы selected
назвал
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.
Переименовал
id: string | ||
activeId: string | ||
items: Item[] | ||
onClick: (item: Item) => void |
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.
наверное onSelect
или onChange
лучше
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.
Сделал onSelect
8fd2384
to
c6b0320
Compare
onMouseUp={renderProps.onMouseUp} | ||
onMouseDown={renderProps.onMouseDown} | ||
cursor="pointer" | ||
mr={props.isLast ? 0 : 6} |
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.
done
renderProps.hover || props.active ? '#ff8c00' : 'transparent' | ||
}`} | ||
children={ | ||
<Typo |
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.
может быть какой-нить Text
заюзать?
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.
заюзал
1bb1c92
to
b0455ae
Compare
b0455ae
to
4c73ad2
Compare
No description provided.