Skip to content

fix: menu regression #295

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 28, 2018
Merged

fix: menu regression #295

merged 4 commits into from
Apr 28, 2018

Conversation

@picodoth
Copy link
Contributor

@picodoth picodoth commented Apr 28, 2018

No description provided.

@picodoth picodoth requested a review from afc163 April 28, 2018 15:01
@afc163
Copy link
Member

afc163 commented Apr 28, 2018

没太明白和 onClick 有啥关系。

@picodoth picodoth mentioned this pull request Apr 28, 2018
Merged
@afc163
Copy link
Member

afc163 commented Apr 28, 2018

ci 挂了

@picodoth
Copy link
Contributor Author

picodoth commented Apr 28, 2018 •
edited
Loading

@afc163

没太明白和 onClick 有啥关系。

https://github.com/react-component/select/blob/master/src/DropdownMenu.jsx#L100
这个地方的onClick被传到<li>去了，加上menuItem自己的onClick，一次click事件会被触发两次，第一次参数是正常的 { item }， 第二次是普通的react event，所以第二次会在https://github.com/react-component/select/pull/295/files#diff-42c78aeee9cd69869523e9f9538de29cL253 挂掉

@picodoth picodoth force-pushed the fix-menu-regression branch from c43e515 to a0a83aa Compare April 28, 2018 15:53
@picodoth
Copy link
Contributor Author

picodoth commented Apr 28, 2018

ci fixed

afc163
afc163 reviewed Apr 28, 2018
View reviewed changes
tests/__snapshots__/Select.tags.spec.js.snap
<li
aria-selected="true"
class="rc-select-dropdown-menu-item rc-select-dropdown-menu-item-selected"
role="menuitem"
Copy link
Member

@afc163 afc163 Apr 28, 2018

Choose a reason for hiding this comment

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

为啥这些 li 没有 role="option"

Copy link
Contributor Author

@picodoth picodoth Apr 28, 2018

Choose a reason for hiding this comment

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

好像是subPopMenu下面的<li>忘记处理了，我加一下

@afc163
Copy link
Member

afc163 commented Apr 28, 2018 •
edited
Loading

见鬼了，本地 ci 是过的，travis 会挂。

好，现在本地也挂了。

@picodoth
Copy link
Contributor Author

picodoth commented Apr 28, 2018

好的 我再看一下

@afc163
Copy link
Member

afc163 commented Apr 28, 2018

没事，我的问题。

@afc163
Copy link
Member

afc163 commented Apr 28, 2018

应该好了。

@codecov-io
Copy link

codecov-io commented Apr 28, 2018 •
edited
Loading

Codecov Report

Merging #295 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #295   +/-   ##
=======================================
  Coverage   99.66%   99.66%           
=======================================
  Files           8        8           
  Lines         297      297           
  Branches       76       76           
=======================================
  Hits          296      296           
  Misses          1        1
Impacted Files Coverage Δ
src/Select.jsx 100% <ø> (ø) ⬆️

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 8b7045c...967eb41. Read the comment docs.

@picodoth
Copy link
Contributor Author

picodoth commented Apr 28, 2018

赞 这回应该可以了 967eb41 我之前以为这些<li>是选中的tag呢，这回加上去了

@picodoth
Copy link
Contributor Author

picodoth commented Apr 28, 2018

我合了啊

@picodoth picodoth merged commit d6adb87 into master Apr 28, 2018
@picodoth picodoth deleted the fix-menu-regression branch April 28, 2018 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afc163 afc163 afc163 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@picodoth @afc163 @codecov-io