feat(MenuToggle): add plain with text variant#6627
Conversation
mcarrano
left a comment
There was a problem hiding this comment.
This looks good. @mcoker @mceledonia I think we covered this before, but I assume we are good with the idea that there is no hover effect on these plain variations?
|
What will it look like in terms of dev time if we add a hover state here to the caret? It will be subtle but consistent with our other icon action patterns. It would have the same colors/states, secondary text color (default) -> primary text color (hover) just to give it a small sense of feedback. I know this isn't what we do on other dropdowns/selects, but it does feel like it needs some kind of state change since other similar components all do. That being said I am sort of on the fence here, if the juice isn't worth the squeeze I think the component will be fine without a hover state.
|
| isExpanded && styles.modifiers.expanded, | ||
| variant === 'primary' && styles.modifiers.primary, | ||
| isPlain && styles.modifiers.plain, | ||
| (isPlain || variant === 'plainText') && styles.modifiers.plain, |
There was a problem hiding this comment.
Can you move the logic of checking the variant to line 39 where isPlain is set.
@mceledonia pretty easy, we can try and get that in this release if you'd like. I'm assuming it should apply to all plain text toggles? Dropdown, options menu, select, etc? |
|
@mcoker |
|
Here is the new issue patternfly/patternfly#4552 |
|
Thanks @nicolethoen . I will add this issue to the next milestone. |
| const { children, className, icon, badge, isExpanded, isDisabled, variant, innerRef, ...props } = this.props; | ||
|
|
||
| const isPlain = variant === 'plain'; | ||
| const isPlainText = variant === 'plainText'; |
There was a problem hiding this comment.
you could combine these into one then line 58 would not have to change.
const isPlain = variant === 'plain' || variant === 'plainText'
There was a problem hiding this comment.
Then i'd have to change the logic on lines 71-72. which I could do. But it's not true that if plaintext then plain. Plaintext also uses the expand/collapse arrow, unlike plain.
mcarrano
left a comment
There was a problem hiding this comment.
This looks good. As stated, we can clean up the hover styles in a future core issue.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #6561
Added a new Menu Toggle example.