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

EuiPortal should be able to use different portal target than document.body based on provided Context #7778

Open
prcdpr opened this issue May 21, 2024 · 8 comments · May be fixed by #7782
Open

Comments

@prcdpr
Copy link

prcdpr commented May 21, 2024

Is your feature request related to a problem? Please describe.
I'm trying to render EuiFlyouts inside the child window opened via window.open. Most of the things work except for Flyouts/Modals as they open in the parent window instead of child window.

I use React portals to render into the child window (via my own <ChildWindow> component).

Describe the solution you'd like

I'd like to do something like the following (pseudocode):

<ChildWindow>
  {(childWindow) => (
     <EuiDomWindowContextProvider target={childWindow.document}>
     
       <EuiFlyout>....</EuiFlyout>

     </EuiDomWindowContextProvider>

  )
</ChildWindow>

EuiDomWindowContextProvider would be a new component that provides window handle via React context to its children.

Existing type EuiPortal, instead of using always the document.body target as in https://github.com/elastic/eui/blob/main/packages/eui/src/components/portal/portal.tsx#L77 it would try to resolve the portal target using the provided value from EuiDomWindowContextProvider first.

As fallback (no context provided) it would use document.body. This would make the change non-breaking.

Describe alternatives you've considered
The only possible workaround right now, would be to disable ownFocus on EuiFlyout and wrap each flyout with custom portals and overlay masks.

Desired timeline
This isn't the most frequent use case for most people but at the same moment is very easy to implement. It would unblock me with using EUI in multi-window Electron app.

@prcdpr
Copy link
Author

prcdpr commented May 21, 2024

EuiWindowEvent would need same treatment too:
https://github.com/elastic/eui/blob/main/packages/eui/src/services/window_event/window_event.ts

It relies on hardcoded window object making it incompatible when rendering via React Portals to child windows.

Meaning that only possible workaround for now is to fork the EUI and apply patches.

@cee-chen
Copy link
Member

cee-chen commented May 21, 2024

Is this something you could resolve via EuiProvider's componentDefaults.EuiPortal.insert API? This prop allows you to configure the insert location of all EuiPortals across the board, including the ones used in flyouts.

https://eui.elastic.co/#/utilities/provider#component-defaults

Regarding EuiWindowEvent, we'd consider taking a componentDefaults configuration for that as well, but it would likely be low priority / we'd want to receive a PR for it.

@stil
Copy link

stil commented May 21, 2024

componentDefaults.EuiPortal.insert wouldn't work, as per documentation it's global, not scoped to the component tree.

Unless it supports nested EuiProvider tags.

Perhaps I'll create a PR for my proposal if I manage to get it working.

(I'm the original issue author just using my private account)

@cee-chen
Copy link
Member

cee-chen commented May 21, 2024

Ah gotcha, no, nested EuiProviders are not supported, sorry - I hadn't realized you were trying to configure this for just one or two flyouts. A PR would be much appreciated!

Edit: To clarify, based on the complexity of the PR, we may not accept it as this is very edge case for Elastic's use cases, but we would certainly review it. If it were a small, elegant, and safe enough change, that would certainly weigh us towards accepting it!

@prcdpr
Copy link
Author

prcdpr commented May 22, 2024

What's the recommended way of using locally forked EUI repository?

Currently my flow is:

On the client repo:

  • yalc add @elastic/eui

It works flawlessly however the yarn build step is quite long and it takes ~3 minutes. Do you have any guidelines how to consume forked EUI but with faster iterations?

@stil stil linked a pull request May 22, 2024 that will close this issue
11 tasks
@tkajtoch
Copy link
Member

Hi @prcdpr! We recommend yarn link-ing your local EUI fork to other projects (see yarn docs).

There isn't a built-in script to build EUI in watch mode as a library (updating the es and lib directories whenever the src directory contents change). You should, however, be able to run one of the following commands to skip the 3-minute build time every time you make a simple change:

  • when importing @elastic/eui as ES modules:
BABEL_MODULES=false NO_COREJS_POLYFILL=true yarn run babel --watch --out-dir=es --extensions .js,.ts,.tsx src
  • when importing @elastic/eui as regular JS (ES5) library:
NO_COREJS_POLYFILL=true yarn run babel --watch --out-dir=lib --extensions .js,.ts,.tsx src

Please note that these commands should be considered a quick and dirty fix to reduce recompilation/prevew time, and you should always run yarn build afterward to emit all of the necessary files.

@prcdpr
Copy link
Author

prcdpr commented May 23, 2024

Watch recompilation commands work however I didn't manage to get it working with yarn link due to local EUI repo having inconsistent node_modules with consuming app causing tons of TS/React errors.

yalc publish && yalc push in combination with yarn run babel --watch works though, even though I need to publish/push manually but at least I don't have to wait 3 minutes for full build.

@stil
Copy link

stil commented Sep 24, 2024

Any chance to reopen #7782 ?
I'm not sure whether it's still considered as something mergeable or is it outside of your scope.
I've recently fixed multiple bugs related to popovers, overlay masks, flyouts as well as proposals of fixes to react-focus-on and react-style-singleton:

theKashey/react-focus-on#96
theKashey/react-style-singleton#12

Currently it works pretty well in my use case, I don't have any outstanding bugs remaining with the components I use.
There might be more components that I didn't test but in my opinion it doesn't have to be zero/one, some support for multiwindowed apps is better than none.

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

Successfully merging a pull request may close this issue.

4 participants