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

ContextMenu & transform #27

Closed
brezetsky opened this issue Aug 25, 2017 · 39 comments
Closed

ContextMenu & transform #27

brezetsky opened this issue Aug 25, 2017 · 39 comments
Assignees

Comments

@brezetsky
Copy link

brezetsky commented Aug 25, 2017

Hi fkhadra,

I got next problem: I have the parent element with style transform. When I add context menu on child element, then its behaviour incorrectly.

Please, find example below.

import React from 'react';
import { ContextMenu, Item, ContextMenuProvider } from 'react-contexify';
import './App.css';

import 'react-contexify/dist/ReactContexify.min.css' 


export default class App extends React.Component<any, any> {
  childrenShow: boolean;
  constructor(props) {
    super(props);
    this.childrenShow = true;

    this.handleMenuSelect = this.handleMenuSelect.bind(this);
    this.handleMenuUnSelect = this.handleMenuUnSelect.bind(this);
    this.handleMenuDelete = this.handleMenuDelete.bind(this);
  }

  render() {
    if ( this.childrenShow ) {
      return (
      <div className="App" style={{marginTop: '200px', transform: 'translate(0)'}}>
        <ContextMenuClass handleMenuSelect={this.handleMenuSelect} handleMenuUnSelect={this.handleMenuUnSelect} handleMenuDelete={this.handleMenuDelete} />
      </div>
      );
  }

    return (
      <div className="App">
      Hiden
    </div>
    );  
  }


  handleMenuUnSelect() {
    this.childrenShow = false;
    console.log('unSelect triggered');
  }

  handleMenuSelect() {
    this.childrenShow = false;
    console.log('select triggered');
  }

  handleMenuDelete() {
    this.childrenShow = false;
    console.log('delete triggered');
    this.forceUpdate();
  }
}

export class ContextMenuClass extends React.Component<any, any> {
  render() {
    return (
      <div>
        <ContextMenuProvider id={'context_menu'}>
        ContextMenu
      </ContextMenuProvider>
      <ContextMenu theme="dark" id={'context_menu'}>
        <Item onClick={this.props.handleMenuUnSelect} disabled>Unselect</Item>
        <Item onClick={this.props.handleMenuSelect} disabled={true}>select</Item>
        <Item onClick={this.props.handleMenuDelete}>Delete</Item>
      </ContextMenu>  
    </div>
  )
  }
}
@fkhadra
Copy link
Owner

fkhadra commented Aug 25, 2017

Hey @brezetsky ,

Indeed, I reproduced the issue there:

Edit Debug issue #27 react-contexify

I'll fix it asap.

@fkhadra fkhadra self-assigned this Aug 25, 2017
@fkhadra fkhadra added the bug label Aug 25, 2017
@fkhadra
Copy link
Owner

fkhadra commented Aug 25, 2017

@brezetsky I don't know if I'll be able to fix it but I found a post which explains what's going on.
BTW, thanks I learned something 😀
Explaination here

@brezetsky
Copy link
Author

brezetsky commented Aug 25, 2017

@fkhadra yes, I read about this. But I think context menu should work from any place on the page. In this case, it should be moved to body and then it will work. Is it possible?

@fkhadra
Copy link
Owner

fkhadra commented Aug 25, 2017

Hmm, it can do the job. Just thinking if there is a way to implement it without relying on findDOMNode. I'm not even sure that findDOMNode can do the trick. I need to find if there is a pattern for this kind of use.

fkhadra added a commit that referenced this issue Aug 25, 2017
@fkhadra
Copy link
Owner

fkhadra commented Aug 25, 2017

Hey @brezetsky ,
I managed to fix it. I'll publish a new version tomorrow. I have to write more tests.

@brezetsky
Copy link
Author

Hey @fkhadra ,

Cool. Thanks.

@brezetsky
Copy link
Author

Hi @fkhadra,

Will you publish it today?

@fkhadra
Copy link
Owner

fkhadra commented Aug 27, 2017

Hey @brezetsky,

I will in a few minutes. I'll let you know

@fkhadra
Copy link
Owner

fkhadra commented Aug 27, 2017

@brezetsky published !

@fkhadra fkhadra closed this as completed Aug 27, 2017
@brezetsky
Copy link
Author

Great! Thanks!

@brezetsky
Copy link
Author

Hi @fkhadra,

it doesn't work now - https://codesandbox.io/s/j2nq05qox9

@fkhadra
Copy link
Owner

fkhadra commented Aug 28, 2017

weird @brezetsky, it's working for me. Cache issue ?

@brezetsky
Copy link
Author

Yes, it was cache but still got problem in my app.
screen shot 2017-08-28 at 11 17 15 am

@fkhadra
Copy link
Owner

fkhadra commented Aug 28, 2017

For your app, maybe delete node_modules and reinstall the package

@vladminsky
Copy link

I have the "Did you forgot to bind the event?" error with 2.1.0 update too.
Can you advice how to handle migration from 2.0.7 to 2.1.0?

Thank you for the lib!

@fkhadra
Copy link
Owner

fkhadra commented Aug 29, 2017

hmm @brezetsky, @vladminsky I think I know why it's happening.

This error means that the event is not bound. Can you show me where in your application you are rendering the ContextMenu component? Did you render it in the root component ?

Anyway, I need to add a rule to check if in that case the event is bound. I'll fix it today.

@vladminsky
Copy link

@fkhadra

Thank you for the quick reply.

Did you render it in the root component?

No. I render ContextMenuProvider and ContextMenu within a subcomponent since it is the only place I need a context menu.

@brezetsky
Copy link
Author

Hey @fkhadra,

I tried to delete node_modules few times. Sometimes, context menu worked form for few elements, but not for all. That's why I continue to use the previous version. Waiting for new version :)

@brezetsky
Copy link
Author

@fkhadra , any updates today?

@fkhadra
Copy link
Owner

fkhadra commented Aug 30, 2017

Hello @brezetsky @vladminsky,

I published a new version could you try it and tell me if it fix your issue please ? Don't forget to clear your cache 😁 .
Thanks

@brezetsky
Copy link
Author

Hello @fkhadra
Still no. For some elements works and for some - no

@fkhadra
Copy link
Owner

fkhadra commented Aug 31, 2017

I don't have a snippet so it's hard to reproduce.Can you provide a snippet, it will be easier for me to see what's going on.

@fkhadra fkhadra reopened this Aug 31, 2017
@Neophius
Copy link

Neophius commented Aug 31, 2017

I'm having the same issue. Assume that ContextMenuComponent is returning
<ContextMenu id="ContextMenuID" />


() => {
return (
<ContextMenuComponent />
.
.
.
<div className="row">
   <ContextMenuProvider id="ContextMenuID"><Button /></ContextMenuProvider>
</div>
)
}

This produces the same event not bound error referenced above.

@fkhadra
Copy link
Owner

fkhadra commented Sep 1, 2017

I'll work on it tonight. I think i will use react portal instead of the solution I put in place.

PS: I hate computer

@brezetsky
Copy link
Author

Will try to make snippet today.
But if you have already a new version - I am ready to test it

@fkhadra
Copy link
Owner

fkhadra commented Sep 2, 2017

im trying to reproduce the bug first

@brezetsky
Copy link
Author

brezetsky commented Sep 3, 2017

hey @fkhadra ,
one of snippets https://codesandbox.io/s/zl9n9496wp

@fkhadra
Copy link
Owner

fkhadra commented Sep 3, 2017

Hey @brezetsky,

Thanks for the snippet i'm working on it

@fkhadra
Copy link
Owner

fkhadra commented Sep 3, 2017

@brezetsky I'm able to reproduce the bug thanks to your snippet. Usually, when I work on a package I use npm link.

With npm link I don't have the bug. I'm trying to understand but this is the first time I encounter such a behavior.

I even tried to pack the lib with npm pack and install it from the packaged archive but still buggy.

screen shot 2017-09-03 at 13 49 21

Edit: diff output nothing also

@brezetsky
Copy link
Author

@fkhadra ,
unfortunately, I didn't work with events. maybe you don't register events for nested elements?
and another question - do you a new context menu for nested elements? Maybe, warning messages are switched off?

@fkhadra
Copy link
Owner

fkhadra commented Sep 3, 2017

@brezetsky I finally found the bug 🎉 !
I must call setState by passing a callback cause I need the previous state. Release the fix in few minutes

@brezetsky
Copy link
Author

@fkhadra really wait it!

@brezetsky
Copy link
Author

brezetsky commented Sep 3, 2017

You can add tests later :)

@brezetsky
Copy link
Author

@fkhadra could I expect the release today? I am sorry that I disturb you, but I want to implement new version of context menu in my project :)

fkhadra added a commit that referenced this issue Sep 3, 2017
fkhadra added a commit that referenced this issue Sep 3, 2017
@fkhadra
Copy link
Owner

fkhadra commented Sep 3, 2017

I pushed the new version. You won't get the "Did you forgot to bind an event" message.

@brezetsky regarding your snippet won't work, only the parent menu will be displayed. To fix it move the <ContextNestedMenuClass /> as follow:

class ContextMenuClass extends React.Component {
  render() {
    const fn = () => { };
    return (
      <div>
        <ContextMenuProvider id={'context_menu'}>
          ContextMenu
      <ContextNestedMenuClass /> -> Remove this
      </ContextMenuProvider>
        <ContextMenu theme="dark" id={'context_menu'}>
          <Item onClick={fn} disabled>Unselect</Item>
          <Item onClick={fn} disabled={true}>select</Item>
          <Item onClick={fn}>Delete</Item>
        </ContextMenu>
{**Put the  <ContextNestedMenuClass /> here instead**}
      </div>
    )
  }
}

Sorry for the late release btw

@fkhadra fkhadra closed this as completed Sep 3, 2017
@brezetsky
Copy link
Author

brezetsky commented Sep 3, 2017

@fkhadra but this Nested context menu worked before for me
I have nested components in my app and for each I have context menu

@brezetsky
Copy link
Author

brezetsky commented Sep 3, 2017

Please, find snippet - https://codesandbox.io/s/zl9n9496wp
I would like to have the different menu for both elements: black and green.

@fkhadra
Copy link
Owner

fkhadra commented Sep 3, 2017

@brezetsky are you sure it was working that way before ?
I made a snippet with an old version and only the parent menu is showing up.
Edit react-contexify debug

Anyway I understand your use case.

@ekilah
Copy link

ekilah commented Sep 24, 2021

I ran into the same issue here, can't tell if it was resolved and regressed from the old discussion above or not, but I was able to fix it by throwing the <Menu> render inside a React Portal:

        {ReactDOM.createPortal(
          this.renderContextMenu(), // this renders the `<Menu>`
          document.body
        )}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants