Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

feat(menu): Vertical Pointing Menu #123

Merged
miroslavstastny merged 6 commits into from
Aug 29, 2018
Merged

feat(menu): Vertical Pointing Menu #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eb7eff2
feat(menu): Vertical Pointing variant
miroslavstastny Aug 23, 2018
44961d8
Merge remote-tracking branch 'origin/master' into feat/menu-vertical-…
miroslavstastny Aug 28, 2018
e240cf6
feat(menu): Horizontal Pointing start variant
miroslavstastny Aug 29, 2018
19eb7f5
Merge branch 'master' into feat/menu-vertical-pointing
kuzhelov Aug 29, 2018
41fba1a
- changelog
miroslavstastny Aug 29, 2018
7d9efb0
Merge remote-tracking branch 'origin/master' into feat/menu-vertical-…
miroslavstastny Aug 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Add accessibility behavior description @kolaps33 ([#74](https://github.com/stardust-ui/react/pull/74))
- Add strict null checks for generated TS types @smykhailov ([#108](https://github.com/stardust-ui/react/pull/108))
- Export themes at `@stardust-ui/react/themes` @levithomason ([#145](https://github.com/stardust-ui/react/pull/145))
- Add support for Menu `vertical pointing` prop @miroslavstastny ([#123](https://github.com/stardust-ui/react/pull/123))

### Documentation
- Add a Quick Start guide @levithomason ([#145](https://github.com/stardust-ui/react/pull/145))
Expand Down
14 changes: 14 additions & 0 deletions docs/src/examples/components/Menu/Variations/MenuExamplePointingStart.shorthand.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import React from 'react'
import { Menu } from '@stardust-ui/react'

const items = [
{ key: 'editorials', content: 'Editorials' },
{ key: 'review', content: 'Reviews' },
{ key: 'events', content: 'Upcoming Events' },
]

const MenuExamplePointingStart = () => (
<Menu defaultActiveIndex={0} items={items} pointing="start" />
)

export default MenuExamplePointingStart
14 changes: 14 additions & 0 deletions docs/src/examples/components/Menu/Variations/MenuExamplePointingStartPrimary.shorthand.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import React from 'react'
import { Menu } from '@stardust-ui/react'

const items = [
{ key: 'editorials', content: 'Editorials' },
{ key: 'review', content: 'Reviews' },
{ key: 'events', content: 'Upcoming Events' },
]

const MenuExamplePointingStartPrimary = () => (
<Menu defaultActiveIndex={0} items={items} pointing="start" type="primary" />
)

export default MenuExamplePointingStartPrimary
14 changes: 14 additions & 0 deletions docs/src/examples/components/Menu/Variations/MenuExampleVerticalPointing.shorthand.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import React from 'react'
import { Menu } from '@stardust-ui/react'

const items = [
{ key: 'editorials', content: 'Editorials' },
{ key: 'review', content: 'Reviews' },
{ key: 'events', content: 'Upcoming Events' },
]

const MenuExampleVerticalPointing = () => (
<Menu defaultActiveIndex={0} items={items} vertical pointing />
)

export default MenuExampleVerticalPointing
14 changes: 14 additions & 0 deletions docs/src/examples/components/Menu/Variations/MenuExampleVerticalPointingEnd.shorthand.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import React from 'react'
import { Menu } from '@stardust-ui/react'

const items = [
{ key: 'editorials', content: 'Editorials' },
{ key: 'review', content: 'Reviews' },
{ key: 'events', content: 'Upcoming Events' },
]

const MenuExampleVerticalPointingEnd = () => (
<Menu defaultActiveIndex={0} items={items} vertical pointing="end" />
)

export default MenuExampleVerticalPointingEnd
20 changes: 20 additions & 0 deletions docs/src/examples/components/Menu/Variations/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,26 @@ const Variations = () => (
description="A menu can point to show its relationship to nearby content."
examplePath="components/Menu/Variations/MenuExamplePointingPrimary"
/>
<ComponentExample
title="Pointing Start"
description="A menu can point up to show its relationship to nearby content."
examplePath="components/Menu/Variations/MenuExamplePointingStart"
/>
<ComponentExample
title="Pointing Start Primary"
description="A menu can point up to show its relationship to nearby content."
examplePath="components/Menu/Variations/MenuExamplePointingStartPrimary"
/>
<ComponentExample
title="Vertical Pointing"
description="A vertical menu can point to start to show its relationship to nearby content."
examplePath="components/Menu/Variations/MenuExampleVerticalPointing"
/>
<ComponentExample
title="Vertical Pointing End"
description="A vertical menu can point to the end to show its relationship to nearby content."
examplePath="components/Menu/Variations/MenuExampleVerticalPointingEnd"
/>
<ComponentExample
title="Underlined"
description="A menu can underline the active element."
Expand Down
7 changes: 5 additions & 2 deletions src/components/Menu/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,11 @@ class Menu extends AutoControlledComponent<any, any> {
/** A menu can adjust its appearance to de-emphasize its contents. */
pills: PropTypes.bool,

/** A menu can point to show its relationship to nearby content. */
pointing: PropTypes.bool,
/**
* A menu can point to show its relationship to nearby content.
* For vertical menu, it can point to the start of the item or to the end.
*/
pointing: PropTypes.oneOfType([PropTypes.bool, PropTypes.oneOf(['start', 'end'])]),

/** The menu can have primary or secondary type */
type: PropTypes.oneOf(['primary', 'secondary']),
Expand Down
7 changes: 5 additions & 2 deletions src/components/Menu/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,11 @@ class MenuItem extends UIComponent<any, any> {
/** A menu can adjust its appearance to de-emphasize its contents. */
pills: PropTypes.bool,

/** A menu can point to show its relationship to nearby content. */
pointing: PropTypes.bool,
/**
* A menu can point to show its relationship to nearby content.
* For vertical menu, it can point to the start of the item or to the end.
*/
pointing: PropTypes.oneOfType([PropTypes.bool, PropTypes.oneOf(['start', 'end'])]),

/** The menu can have primary or secondary type */
type: PropTypes.oneOf(['primary', 'secondary']),
Expand Down
100 changes: 73 additions & 27 deletions src/themes/teams/components/Menu/menuItemStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ const underlinedItem = (color): ICSSInJSStyle => ({
})

const itemSeparator = ({ props, variables }: { props: any; variables }): ICSSInJSStyle => {
const { active, iconOnly, pills, type, underlined, vertical } = props
const { active, iconOnly, pointing, pills, type, underlined, vertical } = props
return {
...(!pills &&
!underlined &&
!(pointing && vertical) &&
!iconOnly && {
'::before': {
position: 'absolute',
Expand All @@ -35,6 +36,56 @@ const itemSeparator = ({ props, variables }: { props: any; variables }): ICSSInJ
}
}

const pointingBeak = ({ props, variables }: { props: any; variables }): ICSSInJSStyle => {
const { pointing, type } = props

let backgroundColor: string
let borderColor: string
let top: string
let borders: ICSSInJSStyle

if (type === 'primary') {
backgroundColor = variables.typePrimaryActiveBackgroundColor
borderColor = variables.typePrimaryBorderColor
} else {
backgroundColor = variables.defaultActiveBackgroundColor
borderColor = variables.defaultBorderColor
}

if (pointing === 'start') {
borders = {
borderTop: `1px solid ${borderColor}`,
borderLeft: `1px solid ${borderColor}`,
}
top = '-1px' // 1px for the border
} else {
borders = {
borderBottom: `1px solid ${borderColor}`,
borderRight: `1px solid ${borderColor}`,
}
top = '100%'
}

return {
'::after': {
visibility: 'visible',
background: backgroundColor,
position: 'absolute',
content: '""',
top,
left: '50%',
transform: 'translateX(-50%) translateY(-50%) rotate(45deg)',
margin: '.5px 0 0',
width: pxToRem(10),
height: pxToRem(10),
border: 'none',
...borders,
zIndex: 2,
transition: 'background .1s ease',
},
}
}

const menuItemStyles = {
root: ({ props, variables }: { props: any; variables: IMenuVariables }): ICSSInJSStyle => {
const { active, iconOnly, pills, pointing, type, underlined, vertical } = props
Expand Down Expand Up @@ -67,6 +118,16 @@ const menuItemStyles = {
color: variables.defaultColor,
}),
...itemSeparator({ props, variables }),
...(pointing &&
Copy link
Contributor

@kuzhelov kuzhelov Aug 23, 2018

Choose a reason for hiding this comment

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

the fact that string value for pointing takes effect only for vertical menu seems to be a bit confusing. Maybe we should consider to introduce analog for horizontal one? At the same time, with that we, probably, will need to aknowledge that for horizontal one default value would be rather end, as menu item's arrow is expected to point down by default:

image

Copy link
Member Author

@miroslavstastny miroslavstastny Aug 28, 2018

Choose a reason for hiding this comment

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

Currently we have no use for horizontal pointing in Teams theme. Instead of adding it now, I would prefer adding it when there is a real requirement for that.

Copy link
Member Author

@miroslavstastny miroslavstastny Aug 28, 2018

Choose a reason for hiding this comment

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

And once we want to add support for top and bottom in pointing, I think it would make more sense to use start and end just for vertical and top and bottom for horizontal.
I understand you would like the properties to be universally valid across variants, but from user's POV it could be confusing. The whole library should consistently use top, end, bottom, start as direction.

Copy link
Contributor

@kuzhelov kuzhelov Aug 28, 2018

Choose a reason for hiding this comment

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

actually, cannot say that I completely agree with that. Let me, please, share my thoughts.

The whole library should consistently use top, end, bottom, start as direction.

While it could be a reasonable choice for some components, for other ones it seems to be not. For the sake of example, suppose that we would have a Layout component similar to the one that currently exists, but with added capability to render elements vertically. In that case start and end, from user's POV, would make perfect sense to me - more than that, it would be much easier for me to change direction from horizontal to vertical,

// client pseudo-code
<Layout start={Foo} end={Bar} />

// changed to
<Layout start={Foo} end={Bar} vertical />

and with that be sure that all the inner composition of elements remains to be valid, i.e.

  • the one that was rendered as start one is still the first one in Layout's column
  • the end one is rendered as last element in column

If it would be about having four properties from two different groups, it would be much more work for me as user, without any good reason (from user's perspective) for that.

Currently we have no use for horizontal pointing in Teams theme. Instead of adding it now, I would prefer adding it when there is a real requirement for that.

Agree that it is generally better to rely on requirements while deciding whether something should be introduced or not, but at the same time I think that when it is about breaking consistency from client's perspective (because, frankly, for me as a consumer it would be a point of sincere astonishment that start and end values of pointing are applied only for vertical menu, while for horizontal one it is easily imaginable how those could be applied), and, at the same time, it is not hard to introduce fix - maybe it would be better to consider to introduce this change even if it is not mentioned in requirements.

Copy link
Member Author

@miroslavstastny miroslavstastny Aug 29, 2018

Choose a reason for hiding this comment

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

Ok, PR updated, horizontal pointing menu now honours start/end as well.

vertical && {
border: '1px solid transparent',
borderTopLeftRadius: `${pxToRem(3)}`,
borderTopRightRadius: `${pxToRem(3)}`,
...(pointing === 'end'
? { borderRight: `${pxToRem(3)} solid transparent` }
: { borderLeft: `${pxToRem(3)} solid transparent` }),
marginBottom: `${pxToRem(12)}`,
}),

':hover': {
color: variables.defaultActiveColor,
Expand Down Expand Up @@ -96,44 +157,29 @@ const menuItemStyles = {
}),
}),
},
...(pointing && {
'::after': {
visibility: 'visible',
background: variables.defaultActiveBackgroundColor,
position: 'absolute',
content: '""',
top: '100%',
left: '50%',
transform: 'translateX(-50%) translateY(-50%) rotate(45deg)',
margin: '.5px 0 0',
width: pxToRem(10),
height: pxToRem(10),
border: 'none',
borderBottom: `1px solid ${variables.defaultBorderColor}`,
borderRight: `1px solid ${variables.defaultBorderColor}`,
zIndex: 2,
transition: 'background .1s ease',
...(type === 'primary' && {
background: variables.typePrimaryActiveBackgroundColor,
borderBottom: `1px solid ${variables.typePrimaryBorderColor}`,
borderRight: `1px solid ${variables.typePrimaryBorderColor}`,
}),
},
}),
...(pointing && !vertical && pointingBeak({ props, variables })),
...(pointing &&
vertical && {
...(pointing === 'end'
? { borderRight: `${pxToRem(3)} solid ${variables.typePrimaryActiveColor}` }
: { borderLeft: `${pxToRem(3)} solid ${variables.typePrimaryActiveColor}` }),
}),
}),
}
},

anchor: ({ props, variables }): ICSSInJSStyle => {
const { active, iconOnly, type, underlined } = props
const { active, iconOnly, pointing, type, underlined, vertical } = props
const { iconsMenuItemSize } = variables

return {
color: 'inherit',
display: 'block',
...(underlined
? { padding: `0 0 ${pxToRem(8)} 0` }
: { padding: `${pxToRem(14)} ${pxToRem(18)}` }),
: pointing && vertical
? { padding: `${pxToRem(8)} ${pxToRem(18)}` }
: { padding: `${pxToRem(14)} ${pxToRem(18)}` }),
cursor: 'pointer',

...(iconOnly && {
Expand Down
3 changes: 2 additions & 1 deletion src/themes/teams/components/Menu/menuStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const solidBorder = (color: string) => ({

export default {
root: ({ props, variables }): ICSSInJSStyle => {
const { iconOnly, fluid, pills, type, underlined, vertical } = props
const { iconOnly, fluid, pointing, pills, type, underlined, vertical } = props
return {
display: 'flex',
...(vertical && {
Expand All @@ -20,6 +20,7 @@ export default {
}),
...(!pills &&
!iconOnly &&
!(pointing && vertical) &&
!underlined && {
...solidBorder(variables.defaultBorderColor),
...(type === 'primary' && {
Expand Down