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

chore: fix aria-* attirbutes #137

Merged
merged 2 commits into from
Apr 28, 2018
Merged

chore: fix aria-* attirbutes #137

merged 2 commits into from
Apr 28, 2018

Conversation

picodoth
Copy link
Contributor

ant-design/ant-design#10095

The idea is to fix the area-* attributes issue and avoid any breaking changes

before:
before

after:
after

@picodoth picodoth requested a review from yesmeck April 27, 2018 12:21
@coveralls
Copy link

coveralls commented Apr 27, 2018

Coverage Status

Coverage increased (+0.008%) to 99.684% when pulling 08bfbd0 on aria-fix into c9c17ed on master.

src/MenuItem.jsx Outdated
'aria-disabled': props.disabled,
...props.attribute,
Copy link
Member

Choose a reason for hiding this comment

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

这个 prop.attribute 好像文档里也没写,不知道哪里用的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我加一下吧,感觉只有在rc-select里有用,而且确实名字也挺含糊的。。

Copy link
Member

Choose a reason for hiding this comment

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

其实感觉也是没什么用?因为之前是不能通过 props 给 li 传 props,但是 #135 就不需要这个属性了?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我看了一下,现在readme.md里Menu.Item的props跟实际上的差很多,这个attribute又感觉优点随机,我加到PropTypes的声明里了

src/MenuItem.jsx Outdated
role: 'option',
'aria-selected': props.isSelected,
}
} else if (attrs.role === 'null') {
Copy link
Member

Choose a reason for hiding this comment

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

null

@@ -316,8 +316,8 @@ export class SubPopupMenu extends React.Component {
);
const domProps = {
className,
role: 'menu',
'aria-activedescendant': '',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

参考: dequelabs/axe-core#179
aria-activedescendant空字符串非法

@@ -181,11 +195,11 @@ export class MenuItem extends React.Component {
}
}

MenuItem.isMenuItem = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

上次忘记改了

@afc163
Copy link
Member

afc163 commented Apr 28, 2018

这个也合掉?

@picodoth picodoth merged commit 625289c into master Apr 28, 2018
@picodoth picodoth deleted the aria-fix branch April 28, 2018 15:32
@zombieJ
Copy link
Member

zombieJ commented Oct 12, 2019

看了一下 attribute 这个在 v3 的 rc-select 没有用到过……感觉是没有用的。

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