feat(menu): Icons #73

miroslavstastny · 2018-08-09T13:46:39Z

API Proposal

Menu Items with icons

MenuItems can now contain icons:

const items = [
  { key: 'home', content: 'Home', icon: { name: 'home', size: 'big' } },
  { key: 'users', content: 'Users', icon: 'users' },
  { key: 'search', icon: 'search' },
]

class MenuExampleWithIconsShorthand extends React.Component {
  render() {
    return <Menu defaultActiveIndex={0} items={items} />
  }
}

Menu can also consist of Icons only (no text):

const items = [
  { key: 'home', icon: {name: 'home', size: 'big'} },
  { key: 'users', icon: 'users' },
  { key: 'search', icon: 'search' },
]

class MenuExampleIconsOnlyShorthand extends React.Component {
  render() {
    return <Menu icons defaultActiveIndex={0} items={items} />
  }
}

Icons only menu intentionally has no spacing between items. In Teams, the free space around the icons is part of the SVG icons. If there is spacing required, it can be achived using IMenuVariables.iconsMenuItemSpacing

Problems

At the moment, it is not possible to create Menu with icons using Children API. We agreed that (for now) only Shorthand API will be supported for Menu.
Icons only menu hover and active states do not match spec as there is no support for hover icons in stardust yet.

TODO

codecov · 2018-08-09T13:47:15Z

Codecov Report

Merging #73 into master will decrease coverage by 0.21%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   86.27%   86.05%   -0.22%     
==========================================
  Files          39       39              
  Lines         663      667       +4     
  Branches       99      102       +3     
==========================================
+ Hits          572      574       +2     
- Misses         88       90       +2     
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Menu/Menu.tsx	`100% <100%> (ø)`	⬆️
src/components/Menu/MenuItem.tsx	`92.3% <60%> (-7.7%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb527f4...bf34c57. Read the comment docs.

mnajdova · 2018-08-09T14:02:05Z

src/components/Menu/MenuItem.tsx

+            {this.props.icon &&
+              Icon.create(this.props.icon, {
+                defaultProps: { xSpacing: !!content ? 'after' : 'none' },
+              })}
            {content}


Should we consider using the Layout component for aligning the content inside the MenuItem? Are there going to be other types of menus with media in the start and end?

Good point. I would suggest leaving it as it is for now and refactor by adding Layout later as new requirements for the component come.

levithomason · 2018-08-15T20:12:21Z

docs/src/examples/components/Menu/Variations/MenuExampleIconsOnly.shorthand.tsx

+  render() {
+    return <Menu icons defaultActiveIndex={0} items={items} />
+  }
+}


This could be stateless for brevity.

levithomason · 2018-08-15T20:12:48Z

docs/src/examples/components/Menu/Variations/MenuExampleWithIcons.tsx

+        <Menu.Item active={activeItem === 'a'} onClick={this.handleItemClick('a')}>
+          <Icon name="home" xSpacing="after" />
+          Home
+        </Menu.Item>


If we are removing shorthand, shouldn't this example be removed?

levithomason · 2018-08-15T20:13:34Z

docs/src/examples/components/Menu/Variations/index.tsx

+      examplePath="components/Menu/Variations/MenuExampleWithIcons"
+    />
+    <ComponentExample
+      title="Vertical Menu with Icons"


Titles should reflect props when possible:
"Vertical Icon" for <Menu vertical icon />

levithomason · 2018-08-15T20:18:46Z

src/components/Menu/MenuItem.tsx

@@ -89,6 +98,10 @@ class MenuItem extends UIComponent<any, any> {
            onClick={this.handleClick}
            {...accessibility.attributes.anchor}
          >
+            {this.props.icon &&


Please use destructuring for consistency.

levithomason · 2018-08-15T20:24:54Z

src/components/Menu/Menu.tsx

@@ -60,6 +63,7 @@ class Menu extends AutoControlledComponent<any, any> {
    'className',
    'defaultActiveIndex',
    'fluid',
+    'icons',


icon and icons props may confuse users. Proposal:

There is no icon prop for Menu yet, propose we use <Menu icon /> to signify the type of the menu.

There is an icon prop already for Menu.Item, propose we use <Menu.item iconOnly /> to signify there is an icon only and no content.

Thoughts?

Agree that icon vs icons on MenuItem is confusing.

But in your proposal icon property means something different in Menu and MenuItem - are we ok with that?

Or we can use iconOnly in Menu as well.

Let's go with iconOnly for now, I will post an RFC about icon type components. There are many considerations beyond the scope of this PR.

miroslavstastny added 2 commits August 9, 2018 15:27

feat(menu): MenuItems can contain icons

8358dba

feat(menu): Icons menu (without any other content)

38c6bc4

miroslavstastny requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners August 9, 2018 13:46

- icon color removed from Menu examples

1be06bb

mnajdova reviewed Aug 9, 2018

View reviewed changes

Merge branch 'master' into feat/menu-icons

3affab9

miroslavstastny added the 🚀 ready for review label Aug 15, 2018

levithomason reviewed Aug 15, 2018

View reviewed changes

levithomason added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Aug 15, 2018

miroslavstastny added 2 commits August 16, 2018 13:01

addressed PR comments

03667a9

Merge remote-tracking branch 'origin/master' into feat/menu-icons

618de47

levithomason approved these changes Aug 16, 2018

View reviewed changes

addressed PR comments - icons -> iconOnly

867876c

miroslavstastny removed the needs author feedback Author's opinion is asked label Aug 17, 2018

miroslavstastny added 2 commits August 17, 2018 11:10

Merge remote-tracking branch 'origin/master' into feat/menu-icons

63f36fe

changelog

bf34c57

miroslavstastny merged commit a450551 into master Aug 17, 2018

miroslavstastny mentioned this pull request Aug 23, 2018

feat: add Menu Icons variation stardust-ui/react-old#121

Closed

levithomason deleted the feat/menu-icons branch October 29, 2019 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(menu): Icons #73

feat(menu): Icons #73

miroslavstastny commented Aug 9, 2018 •

edited by pompomon

codecov bot commented Aug 9, 2018 •

edited

mnajdova Aug 9, 2018

miroslavstastny Aug 13, 2018

mnajdova Aug 13, 2018

levithomason Aug 15, 2018

levithomason Aug 15, 2018

levithomason Aug 15, 2018

levithomason Aug 15, 2018

levithomason Aug 15, 2018

miroslavstastny Aug 16, 2018

levithomason Aug 16, 2018

feat(menu): Icons #73

feat(menu): Icons #73

Conversation

miroslavstastny commented Aug 9, 2018 • edited by pompomon

API Proposal

Menu Items with icons

Problems

TODO

codecov bot commented Aug 9, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miroslavstastny commented Aug 9, 2018 •

edited by pompomon

codecov bot commented Aug 9, 2018 •

edited