feat(menu): pointing && vertical menu redlining #1116
Conversation
jaanus03
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
|
I am just wondering, do we really have purple left border in hoc theme? |
Hey. Good point. Probably not, but let me confirm. |
Codecov Report
@@ Coverage Diff @@
## master #1116 +/- ##
==========================================
+ Coverage 82.07% 82.09% +0.01%
==========================================
Files 724 724
Lines 8663 8661 -2
Branches 1236 1234 -2
==========================================
Hits 7110 7110
+ Misses 1536 1534 -2
Partials 17 17
Continue to review full report at Codecov.
|
packages/react/src/themes/teams/components/Menu/menuItemStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Menu/menuItemStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Menu/menuItemStyles.ts
Outdated
Show resolved
Hide resolved
You were right. No indicator in the HC theme. Removed it. |
| @@ -7,6 +7,8 @@ export default (siteVars: any): Partial<MenuVariables> => ({ | |||
| focusedBorder: `solid ${pxToRem(1)} ${siteVars.colors.white}`, | |||
| focusedBackgroundColor: 'transparent', | |||
|
|
|||
| primaryActiveBorderColor: siteVars.brand06, | |||
There was a problem hiding this comment.
I would consider renaming this to something without "primary" in it. It might make people think they need the primary prop, which they don't.
There was a problem hiding this comment.
Correct. Renamed it.
| ? pointing === 'end' | ||
| ? { borderRight: `${pxToRem(3)} solid ${v.primaryActiveBorderColor}` } | ||
| : { borderLeft: `${pxToRem(3)} solid ${v.primaryActiveBorderColor}` } | ||
| ? !isFromKeyboard && { |
There was a problem hiding this comment.
I find this ternary operator difficult to read. Would it be easier to scan if this were flat, like
...(pointing && vertical && !isFromKeyboard && {
styles here
}),
...(pointing && !vertical && {
styles here
}),
| @@ -9,6 +9,7 @@ type MenuItemPropsAndState = MenuItemProps & MenuItemState | |||
| export const verticalPillsBottomMargin = pxToRem(5) | |||
| export const horizontalPillsRightMargin = pxToRem(8) | |||
| export const verticalPointingBottomMargin = pxToRem(12) | |||
| export const verticalPointingBottomMarginFocus = pxToRem(13) | |||
There was a problem hiding this comment.
I think you can remove this
jaanus03
force-pushed
the
jaanusp/vertical-pointing-menu-redlining
branch
from
474ee70
to
fd23cb3
Compare
jaanus03
force-pushed
the
jaanusp/vertical-pointing-menu-redlining
branch
from
5f1482c
to
787c4e4
Compare
delete-merged-branch
bot
deleted the
jaanusp/vertical-pointing-menu-redlining
branch
This PR is a minor update to vertical && pointing menu styles.
It removes bordered corners, straightens the "pointing" indicator and adds focus styles.