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.

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

Bubbling event from OutPortal #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bubbling event from OutPortal #34

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de346c9
Bubble all events from InPortal to OutPortal
alessandro308 Mar 13, 2022
7e48746
WIP: Make it clearer where events are firing
pimterry Mar 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ interface PortalNodeBase<C extends Component<any>> {
// If an expected placeholder is provided, only unmount if that's still that was the
// latest placeholder we replaced. This avoids some race conditions.
unmount(expectedPlaceholder?: Node): void;

}
export interface HtmlPortalNode<C extends Component<any> = Component<any>> extends PortalNodeBase<C> {
element: HTMLElement;
Expand Down Expand Up @@ -159,8 +160,25 @@ class InPortal extends React.PureComponent<InPortalProps, { nodeProps: {} }> {
this.addPropsChannel();
}

componentDidUpdate() {
componentDidUpdate(previousProps: InPortalProps) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the right place to handle this, which I think might cause the current story behaviour. This method will fire when the InPortal's props change, but that doesn't really happen (the only InPortal prop in this example is node, which is memoized).

Adding some logging & playing with this, it seems that the if test here is always false in this story, so the event redirection never takes effect.

I think you probably want to put this in the portalNode.mount/unmount methods - those always get called any time the node is moved from one place to another, so that's a good place to set up & tear down any event redirection.

Copy link
Contributor

@adri1wald adri1wald May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hey I'm going to try to implement this. It's a critical requirement for our usecase of react-reverse-portal as we have functionality where the user can drop over elements coming out of an OutPortal, but the dnd handlers are on the parent of the OutPortal

Edit: I can't seem to figure it out :')

Edit 2: I can get the event to the OutPortal parent but only by cloning it, like

e.stopPropagation()
if (parent) {
  parent.dispatchEvent(new e.constructor(e.type, e))
}

This removes necessary properties for the drag event though. Without cloning I get Failed to execute 'dispatchEvent' on 'EventTarget': The event is already being dispatched.

Copy link
Member

Choose a reason for hiding this comment

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

Ok! If you want to take a shot at this I'm happy to review it and merge it if you can get something working.

This removes necessary properties for the drag event though

Which properties specifically? Can we just clone them manually?

Could we just manually clone all event properties with Object.assign? Could use some kind of blocklist to drop any known-problematic ones, but I would expect that to work for most cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey sorry I never followed up on this. In the end I moved the parent of the OutPortal, which carried the dnd handlers, into the InPortal and that solved my problem.

this.addPropsChannel();
if(previousProps.node.element !== this.props.node.element){
Object.keys(window).forEach(key => {
if (/^on/.test(key)) {
const eventType = key.slice(2);
this.props.node.element.addEventListener(eventType, this.onEventHandler);
if(previousProps.node.element){
previousProps.node.element.removeEventListener(eventType, this.onEventHandler);
}
}
});

}
}

onEventHandler(e:any){
e.stopPropagation();
this.props.node.element.dispatchEvent(e);
}

render() {
Expand Down
44 changes: 44 additions & 0 deletions stories/html.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,5 +349,49 @@ storiesOf('Portals', module)
</div>;
}

return <MyComponent componentToShow='component-a' />
}).add('Events bubbling from PortalOut', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This example doesn't seem to work as I was expecting. On my machine, I can still see the events bubbling up into the InPortal.

I've pushed a WIP commit here which extends this story slightly to demonstrate the issue. When I click the 'A' element it shows the 'OutPortal' wrapper event, but when I click the 'expensive' element, it shows the 'InPortal' wrapper event.

With the change we're aiming for, I think it should always show the OutPortal event, right? No events should ever bubble out of the InPortal.

const MyExpensiveComponent = () => <div onMouseDown={() => console.log('expensive')}>expensive!</div>;

const MyComponent = () => {
const portalNode = React.useMemo(() => createHtmlPortalNode(), []);

return <div>
{/*
Create the content that you want to move around.
InPortals render as normal, but to detached DOM.
Until this is used MyExpensiveComponent will not
appear anywhere in the page.
*/}
<InPortal node={portalNode}>
<MyExpensiveComponent
// Optionally provide props to use before this enters the DOM
myProp={"defaultValue"}
/>
</InPortal>

{/* ... The rest of your UI ... */}

{/* Pass the node to whoever might want to show it: */}
<ComponentA portalNode={portalNode} />
</div>;
}

const ComponentA = (props) => {
return <div
onClick={() => alert('Div clicked')}
onMouseDown={() => console.log('Mouse Down')}
onMouseEnter={() => console.log('Mouse enter')}
>
{/* ... Some more UI ... */}

A:

<OutPortal
node={props.portalNode} // Show the content from this node here
/>
</div>;
}

return <MyComponent componentToShow='component-a' />
});