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
fix: dropdown menu item #39368
fix: dropdown menu item #39368
Conversation
Disable dropdown menu item when type is equal to danger
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #39368 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 556 556
Lines 9602 9624 +22
Branches 2714 2722 +8
=========================================
+ Hits 9602 9624 +22
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
components/menu/MenuItem.tsx
Outdated
@@ -66,7 +66,7 @@ export default class MenuItem extends React.Component<MenuItemProps> { | |||
{...rest} | |||
className={classNames( | |||
{ | |||
[`${prefixCls}-item-danger`]: danger, | |||
[`${prefixCls}-item-danger`]: this.props.disabled ? false : danger, | |||
[`${prefixCls}-item-only-child`]: (icon ? childrenLength + 1 : childrenLength) === 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can solve the problem here by increasing the css selector weights.
v4.x
ant-design/components/dropdown/style/index.less
Lines 224 to 226 in f4d0116
&-disabled { | |
color: @disabled-color; | |
cursor: not-allowed; |
v5.x
ant-design/components/dropdown/style/index.tsx
Lines 350 to 352 in 122b3f1
'&-disabled': { | |
color: colorTextDisabled, | |
cursor: 'not-allowed', |
what do you think?
- &-disabled {
+ &&-disabled {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding !important
statement to color
and background-color
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of !important
is bad, in 5.x it can be changed to this
- '&-disabled': {
+ [`&${menuCls}-item-disabled`]: {
color: colorTextDisabled,
and switch the order of returns
return [
+ genStatusStyle(dropdownToken),
genBaseStyle(dropdownToken),
genButtonStyle(dropdownToken),
- genStatusStyle(dropdownToken),
];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just my suggestion, still need official help to review @afc163
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Wxh16144's suggestion is what I think too.
|
This comment was marked as resolved.
This comment was marked as resolved.
components/dropdown/style/index.tsx
Outdated
@@ -456,9 +456,9 @@ export default genComponentStyleHook( | |||
dropdownEdgeChildPadding: paddingXXS, | |||
}); | |||
return [ | |||
genStatusStyle(dropdownToken), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to remain the order because genBaseStyle
should be at first.
Maybe problem is solved without changing order since the priority has been raised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to remain the order because
genBaseStyle
should be at first. Maybe problem is solved without changing order since the priority has been raised?
I've also tested this and it still doesn't fix the problem without reordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should provide higher priority instead of reordering, since it may cause other issue related with order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes without reordering the functions
Remove reordering functions by increasing `disable` class on line 350 CSS specificity
@@ -347,7 +347,7 @@ const genBaseStyle: GenerateStyle<DropdownToken> = (token) => { | |||
}, | |||
}, | |||
|
|||
'&-disabled': { | |||
[`&${menuCls}-item-disabled${menuCls}-item-disabled`]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[`&${menuCls}-item-disabled${menuCls}-item-disabled`]: { | |
[`&-disabled, &-disabled${menuCls}-item-danger`]: { |
How about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it didn't work
Fixed in #39434 |
@Wxh16144 你改过V4, 把把关 |
This comment was marked as outdated.
This comment was marked as outdated.
@Wxh16144 进展如何 |
我认为还是最开始的解决方案 OK 一些, 查看了 ant-design/components/menu/style/theme.tsx Lines 67 to 69 in 24c066e
重新提交了一个 PR #39904 |
Disable dropdown menu item when type is equal to danger.
[中文版模板 / Chinese template]
🤔 This is a ...
🔗 Related issue link
💡 Background and solution
Problem: When we set the dropdown menu item properties
danger
anddisabled
totrue
, the disabled styling doesn't apply. As shown in below code sandbox 👇https://codesandbox.io/embed/antd-reproduction-template-forked-ip8ypb?fontsize=14&hidenavigation=1&theme=dark
Solution: check if dropdown menu item is disabled or not
If yes -> we are not going to apply danger staying, same as how button handles it (shown in code sandbox).
📝 Changelog
This file "components/menu/MenuItem.tsx" is updated.
☑️ Self-Check before Merge