-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Add Popover #29084
Components: Add Popover #29084
Conversation
Size Change: +499 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
Interesting :). Well, we can do some work to make the new Will take a look later today! <3 |
@sarayourfriend Haiii!
I guess that's the tricky part when components depend on another's context 🙈 If we wanted to make a "cleaner split", we can make @sarayourfriend What do you think? Re: Popover styles I took it for a spin, and it looked like it was working okay. There were some styles coming from the |
6005924
to
c31a492
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 from me. Thanks @sarayourfriend !
Following up on our 🍐 session. Given how complex the correct Popover
is, we think we should attempt to adapt it in a separate PR.
@sarayourfriend Haiii! I'm seeing a type (?) issue for Not sure what's going on there 🤔 |
@ItsJonQ Have you |
@sarayourfriend I thought I did... I tried again, and it looks like it's okay now :). In other news... some of the E2E tests are failing with this error:
I wonder what's up 🤔 |
@sarayourfriend I'm having trouble replicating this locally (my E2E tests are giving me other issues). For debugging purposes, I'm going to try out various versions of |
@sarayourfriend I have a hunch!! Checkout this line in https://github.com/ItsJonQ/g2/blob/main/packages/context/src/context-connect.js#L54 WrappedComponent[INTERPOLATION_CLASS_NAME] = `ic-${hash(displayName)}`; In However, in the case of Just a hunch! In browsing the local files that use |
Thanks for the fix @sarayourfriend !! I don't think this PR relies on this update anymore (although, it would be handy to have) |
Description
This PR adds Popover.
How has this been tested?
Unit tests and storybook
Types of changes
New features
Checklist: