Skip to content
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

support custom arrow icon in vertical and inline mode. #182

Merged
merged 20 commits into from
Aug 22, 2018

Conversation

HeskeyBaozi
Copy link
Contributor

向外暴露arrowIcon接口来自定义菜单图标。
只限于垂直和内联模式。
以便未来使用svg图标。

@coveralls
Copy link

coveralls commented Jul 24, 2018

Coverage Status

Coverage increased (+0.008%) to 99.685% when pulling 34356d7 on HeskeyBaozi:master into d7eaed4 on react-component:master.

@@ -0,0 +1,294 @@
@menuPrefixCls: rc-menu;
Copy link
Contributor

Choose a reason for hiding this comment

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

这里好像跟 index.less 只有三行不一样的代码,是不是应该 @import 'index' 然后覆盖修改的样式。

Copy link
Member

@yesmeck yesmeck Jul 25, 2018

Choose a reason for hiding this comment

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

没必要新增这个文件,支持覆盖 arrowIcon,但是不需要管覆盖的 arrowIcon 的样式。

Copy link
Member

Choose a reason for hiding this comment

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

这个文件要去掉,自定义图标的样式这里不管。

console.log(info);
}

const animation = {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里重命名一下吧,好奇怪竟然不报错。

import 'rc-menu/assets/custom-arrow-icon.less';
import animate from 'css-animation';

const getSvgIcon = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

这个地方能不能通过 example 页面的一个 toggle 来做,否则又是大量重复代码。

Copy link
Member

Choose a reason for hiding this comment

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

简单的功能,没必要单独写个 demo

src/SubMenu.jsx Outdated
<i className={`${prefixCls}-arrow`} />
<i className={`${prefixCls}-arrow`}>
{arrowIcon}
</i>
Copy link
Member

Choose a reason for hiding this comment

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

一样的不需要套在 <i> 里。

@ant-design-bot
Copy link

ant-design-bot commented Jul 30, 2018

Deploy preview for rc-menu ready!

Built with commit 34356d7

https://deploy-preview-182--rc-menu.netlify.com

yesmeck
yesmeck previously approved these changes Jul 30, 2018
@@ -0,0 +1,294 @@
@menuPrefixCls: rc-menu;
Copy link
Member

Choose a reason for hiding this comment

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

这个文件要去掉,自定义图标的样式这里不管。

@yesmeck yesmeck dismissed their stale review July 30, 2018 04:23

点错了

src/SubMenu.jsx Outdated
@@ -474,7 +479,7 @@ export class SubMenu extends React.Component {
title={typeof props.title === 'string' ? props.title : undefined}
>
{props.title}
<i className={`${prefixCls}-arrow`} />
{arrowIcon || <i className={`${prefixCls}-arrow`}/>}
Copy link
Member

Choose a reason for hiding this comment

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

/> 前要有空格。。

@HeskeyBaozi
Copy link
Contributor Author

graph

README.md Outdated
<td>(props: MenuItemProps | SubMenuProps) => ReactNode</td>
<th></th>
<td>specific the arrow icon.</td>
</tr>
Copy link
Member

Choose a reason for hiding this comment

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

arrowIcon?

Copy link
Member

Choose a reason for hiding this comment

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

itemIcon?

@@ -474,7 +483,7 @@ export class SubMenu extends React.Component {
title={typeof props.title === 'string' ? props.title : undefined}
>
{props.title}
<i className={`${prefixCls}-arrow`} />
{icon || <i className={`${prefixCls}-arrow`} />}
Copy link
Member

Choose a reason for hiding this comment

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

这个 icon 应该只给 MenuItem 用吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

也包括SubMenu

README.md Outdated
<td>expandIcon</td>
<td>(props: SubMenuProps) => ReactNode</td>
<th></th>
<td>specific the menu item icon.</td>
Copy link
Member

Choose a reason for hiding this comment

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

SubMenu

src/SubMenu.jsx Outdated
@@ -529,7 +540,10 @@ export class SubMenu extends React.Component {
}
}

const connected = connect(({ openKeys, activeKey, selectedKeys }, { eventKey, subMenuKey }) => ({
const connected = connect((
Copy link
Member

Choose a reason for hiding this comment

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

无关的提交

src/Menu.jsx Outdated
static contextTypes = {
itemIcon: PropTypes.func,
expandIcon: PropTypes.func,
}
Copy link
Member

Choose a reason for hiding this comment

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

context 应该去掉?

Copy link
Contributor Author

@HeskeyBaozi HeskeyBaozi Aug 13, 2018

Choose a reason for hiding this comment

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

师兄你是对的。
用到的两处地方DropdownSelect都可以通过传itemIconexpandIcon<MenuItem />即可。

我一开始的想法就是透过原本的rc-dropdown和rc-select传入到里层的rc-menu,才用的context。现在发现没有必要。
这样之后rc-select可能还会有个pr。

@@ -63,6 +63,8 @@ export class SubMenu extends React.Component {
store: PropTypes.object,
mode: PropTypes.oneOf(['horizontal', 'vertical', 'vertical-left', 'vertical-right', 'inline']),
manualRef: PropTypes.func,
itemIcon: PropTypes.oneOfType([PropTypes.func, PropTypes.node]),
Copy link
Member

Choose a reason for hiding this comment

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

这行是多余的?

@@ -366,6 +368,8 @@ export class SubMenu extends React.Component {
prefixCls: props.rootPrefixCls,
id: this._menuId,
manualRef: this.saveMenuInstance,
itemIcon: props.itemIcon,
Copy link
Member

Choose a reason for hiding this comment

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

多余的?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SubMenu也类似Menu一样,需要ItemIcon传给children中的MenuItem。
若去掉则整个Menu只有第一层有图标,SubMenu中的MenuItem都没有图标。

@yesmeck
Copy link
Member

yesmeck commented Aug 16, 2018

看下测试挂了。

@yesmeck
Copy link
Member

yesmeck commented Aug 17, 2018

master 上是好的,看下有什么区别

@yesmeck
Copy link
Member

yesmeck commented Aug 17, 2018

可以了,rebase 下 master

@yesmeck
Copy link
Member

yesmeck commented Aug 21, 2018

@picodoth 看下

README.md Outdated
<td>itemIcon</td>
<td>ReactNode | (props: MenuItemProps) => ReactNode</td>
<th></th>
<td>specific the menu item icon.</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify icon for menu item.

README.md Outdated
<td>expandIcon</td>
<td>ReactNode | (props: SubMenuProps & { isSubMenu: boolean }) => ReactNode</td>
<th></th>
<td>specific the menu item icon.</td>
Copy link
Contributor

@picodoth picodoth Aug 22, 2018

Choose a reason for hiding this comment

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

Specify expanded icon for menu item submenu title

README.md Outdated
<td>itemIcon</td>
<td>ReactNode | (props: MenuItemProps) => ReactNode</td>
<th></th>
<td>specific the menu item icon.</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify icon for menu item.

README.md Outdated
<td>itemIcon</td>
<td>ReactNode | (props: SubMenuProps & { isSubMenu: boolean }) => ReactNode</td>
<th></th>
<td>specific the menu item icon.</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

同上

@@ -30,6 +30,7 @@ export class MenuItem extends React.Component {
multiple: PropTypes.bool,
isSelected: PropTypes.bool,
manualRef: PropTypes.func,
itemIcon: PropTypes.oneOfType([PropTypes.func, PropTypes.node]),
Copy link
Contributor

Choose a reason for hiding this comment

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

这里不需要 expandIcon 么

Copy link
Contributor

Choose a reason for hiding this comment

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

知道了 expandedicon 只在 submenu 上生效

if (typeof this.props.expandIcon === 'function') {
icon = React.createElement(
this.props.expandIcon,
{ ...this.props }
Copy link
Contributor

Choose a reason for hiding this comment

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

这里为啥需要传 props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里可以让使用者根据mode等属性使用不同的图标

Copy link
Contributor

Choose a reason for hiding this comment

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

没懂,举个🌰?

Copy link
Contributor Author

@HeskeyBaozi HeskeyBaozi Aug 22, 2018

Choose a reason for hiding this comment

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

function expandIcon(props) {
  return getSvgIcon({
    position: 'absolute',
    right: '1rem',
    color: props.mode === 'vertical' ? 'lightblue' : 'hotpink',
    transform: `rotate(${props.isOpen ? 90 : 0}deg)`,
  });
}

这里是根据modeisOpen确定不同的样式。

Copy link
Contributor

Choose a reason for hiding this comment

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

是啊,那不需要吧, {...this.props} this.props.expandedIcon 已经包含这个信息了

@picodoth
Copy link
Contributor

@HeskeyBaozi

@yesmeck yesmeck merged commit 07b4ccc into react-component:master Aug 22, 2018
@yesmeck
Copy link
Member

yesmeck commented Aug 23, 2018

7.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants