Skip to content
This repository has been archived by the owner on Sep 1, 2020. It is now read-only.

In MenuItem, don't override className prop, but extend it #293

Closed
wuppious opened this issue Aug 29, 2019 · 5 comments · Fixed by #294
Closed

In MenuItem, don't override className prop, but extend it #293

wuppious opened this issue Aug 29, 2019 · 5 comments · Fixed by #294

Comments

@wuppious
Copy link
Contributor

Currently, you can give MenuItem classNames through the attributes prop. These are then applied to the element, but also overriding any className given as a direct prop.

const { disabled, divider, children, attributes, selected } = this.props;
const menuItemClassNames = cx(cssClasses.menuItem, attributes.className, {
[cx(cssClasses.menuItemDisabled, attributes.disabledClassName)]: disabled,
[cx(cssClasses.menuItemDivider, attributes.dividerClassName)]: divider,
[cx(cssClasses.menuItemSelected, attributes.selectedClassName)]: selected
});
return (
<div
{...attributes} className={menuItemClassNames}

The reason why this might be problematic, is that some css-in-js libraries such as styled-components apply an automatically generated className to the props, which then gets overridden in the render method.

My suggestion would be to accept this className prop and extend it instead of overriding.

@wuppious
Copy link
Contributor Author

Meanwhile, a workaround is to wrap the MenuItem into a functional component and apply the generated className to the attributes.

const MenuItemWithClassName = props => (
  <MenuItem {...props} attributes={{ className: props.className }} />
);

@vkbansal
Copy link
Owner

Good catch!. This library was written when CSS-in-JS libraries were not a thing.

Now that they are a thing, it should be taken care of. Will you be willing to submit a PR?

@wuppious
Copy link
Contributor Author

I can have a look during the weekend.

@wuppious
Copy link
Contributor Author

wuppious commented Sep 1, 2019

@vkbansal PR is done, however it fails due to Code Climate config error. I don't believe I can affect that, so please check into it.

@vkbansal
Copy link
Owner

vkbansal commented Sep 1, 2019 via email

vkbansal pushed a commit that referenced this issue Sep 3, 2019
…ing (#294)

* fix: linter errors and warnings

* feat: extend MenuItem className correctly

Closes #293
erjstar added a commit to erjstar/react-contextmenu that referenced this issue Apr 28, 2022
…ing (#294)

* fix: linter errors and warnings

* feat: extend MenuItem className correctly

Closes vkbansal/react-contextmenu#293
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants