Skip to content

Conversation

@y1j2x34
Copy link
Contributor

@y1j2x34 y1j2x34 commented Jun 29, 2023

Summary

See also #6882

QA

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 29, 2023

💚 CLA has been signed

@cee-chen
Copy link
Contributor

cee-chen commented Jul 7, 2023

Hey @y1j2x34 - just wanted to say apologies for not responding sooner, I promise we haven't forgotten your PR! Incredible work on this by the way, it's really impressive! :)

The main thing change I'll be making on top of it is that we want to expand the functionality of <EuiProvider> to accept more configurations for components than just EuiPortal, so the singular top-level portal prop is a little too limiting. This is the current API structure/architecture we have in mind:

<EuiProvider
  componentDefaults={{
    EuiPortal: { insert: { sibling: someNode, position: 'after' } },
    EuiFocusTrap: { crossFrame: true },
    // ... etc
  }}
>
  <App />
</EuiProvider>

Currently, that work is in review in #6923. Once that PR merges into the feature branch, I'll go ahead and update this PR to point at that feature branch, and then tweak your PR behavior to use the new provider and add some Cypress tests.

FYI for timing, we're probably looking at around 1-2 weeks for this to land in main and then be released. Hope that helps - thank you again for kickstarting this work off, it was seriously incredibly helpful and your contribution could not be more appreciated!

@cee-chen
Copy link
Contributor

Hey @y1j2x34 - it looks like your fork doesn't have permissions for EUI maintainers to push to it, so I'm going to go ahead and close this PR in favor of #6941. It still has a good amount of your original code in it however. Thank you again!

@cee-chen cee-chen closed this Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants