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

Add EuiWindowProvider for multi-window support #7782

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

stil
Copy link

@stil stil commented May 22, 2024

Summary

Closes #7778

The main idea in this PR is to enable multi-windowed React development with EUI, for example in Electron.
The change is purely bug-fixing and does not introduce any UI changes.

I want to enable multi-windowed support by using React Portals to render into different DOM windows.
The problem is that, in many places, EUI operates on global window or document object which I aim to fix.

It's not a completely novel idea, something similar can be found in FluentUI: https://github.com/microsoft/fluentui/tree/master/packages/react-window-provider

New <EuiWindowProvider> component is exposing current window object for its children elements.

I'm marking it as draft so early-on you can have a look what's inside and perhaps immediately tell that it's not going to be merged, then I'll continue work internally.

The change is completely backwards compatible and does not introduce any behavior or UI changes.

The most important is that with these changes Flyouts work in child windows!

Let me know wheter this is something you'd consider at all for merging (assuming that all potential issues with PR are fixed).

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • Updated the Figma library counterpart

Copy link

cla-checker-service bot commented May 22, 2024

💚 CLA has been signed

Copy link

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

Copy link

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

Copy link

❌ We're automatically closing this PR due to lack of activity. Please comment if you feel this was done in error.

@stil
Copy link
Author

stil commented Sep 24, 2024

Please reopen - I'm still working on fixes and now it's in better state than before with many bugs fixed.

@cee-chen
Copy link
Member

Re-opening! Apologies @stil, we hadn't even realized this PR was in the queue - we don't get automatically pinged/notified for draft PRs unless they open.

@cee-chen cee-chen reopened this Sep 24, 2024
Copy link

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

@cee-chen
Copy link
Member

buildkite test this

@cee-chen
Copy link
Member

As a heads up you will need to sign the Elastic CLA with the email address associated with the git commits (use git log to see that email address).

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen
Copy link
Member

@stil Any update on signing https://www.elastic.co/contributor-agreement? We cannot move forward with merging this in without it signed with the emails associated with the commits in this PR, unfortunately.

@stil
Copy link
Author

stil commented Sep 30, 2024

Sorry I thought you receive a notification when signed.

I have already signed the agreement.

@stil
Copy link
Author

stil commented Oct 1, 2024

I'll complete rest of the steps by the end of week and mark it for review.

@stil
Copy link
Author

stil commented Oct 6, 2024

Current status:

My suggestion would be to wait a bit for the 3rd party libs MRs to be merged and released, without these we cannot really merge this MR as the behavior is a bit buggy.

When third party libs are fixed, I'm going to:

  • Update the changelog entry with updated package.json dependency on react-focus-on.
  • Ensure that merged bugfix in react-style-singleton works out of the box with EUI. The fix might be either via exposed WindowProvider from react-style-singleton or via internal window object passing in which case no modifications are going to be done. This is still being worked on by react-style-singleton author.

Anyway in the current state, the fixes for EUI, react-focus-on and react-style-singleton work great in my usecase even though it was a bit painful to track bugs across the libraries 😅

@stil stil marked this pull request as ready for review October 6, 2024 21:20
@stil stil requested a review from a team as a code owner October 6, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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