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

Fix scoping in the Composite context #3799

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

Conversation

DaniGuardiola
Copy link
Contributor

Fixes #3768

Copy link

changeset-bot bot commented May 17, 2024

⚠️ No Changeset found

Latest commit: 4515b34

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codesandbox-ci bot commented May 17, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@DaniGuardiola
Copy link
Contributor Author

@diegohaz any quick way to create a starter codesandbox using this PR's version?

@diegohaz
Copy link
Member

@diegohaz any quick way to create a starter codesandbox using this PR's version?

I'm not sure. It used to be generated automatically, but it stopped working suddenly a while ago.

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

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

Thanks @DaniGuardiola!

The changes make sense and align with how other composite components are implemented. It would be nice to have a simple example of what you're trying to do on WordPress that triggered this issue.

@DaniGuardiola
Copy link
Contributor Author

Thanks @diegohaz, here's the example: https://stackblitz.com/edit/vitejs-vite-ihjzqp?file=src%2FApp.tsx

This should work with this PR's version.

@diegohaz
Copy link
Member

diegohaz commented May 20, 2024

Thanks @diegohaz, here's the example: https://stackblitz.com/edit/vitejs-vite-ihjzqp?file=src%2FApp.tsx

This should work with this PR's version.

Thanks! Would you mind turning that into an example with tests (without a readme.md file) for this PR?

@DaniGuardiola
Copy link
Contributor Author

@diegohaz of course, I actually already have that locally since I needed to test it, I just didn't push it haha. Didn't realize you were referring to that. I will push it in a bit.

@DaniGuardiola
Copy link
Contributor Author

@diegohaz pushed, let me know if the test is adequate. Here's a demo of it:

Kapture.2024-05-20.at.23.08.36.mp4

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.

Composite: two-dimensional arrow navigation with MenuButtons
2 participants