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

feat(popover): add cdk events service #217

Conversation

pawel-twardziak
Copy link
Contributor

Description

feat(popover): add cdk events service

Copy link

vercel bot commented Dec 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
blocks-showcase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 7:37am
primitives ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 7:37am
radix-astro-doc ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 7:37am
shadcn-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 7:37am
taxonomy ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 7:37am

Copy link

vercel bot commented Dec 21, 2024

@pawel-twardziak is attempting to deploy a commit to the Radix-angular Team on Vercel.

A member of the Team first needs to authorize it.

@pawel-twardziak
Copy link
Contributor Author

pawel-twardziak commented Dec 21, 2024

@pimenovoleg I've just reproduced the issue you talked about before :) the scrollbar appears and disappears quickly making the popover position wrongly computed. Interesting issue.

Primitives._.Popover.-.Docs.Storybook.-.Google.Chrome.2024-12-21.18-47-57.mp4

@pawel-twardziak
Copy link
Contributor Author

the message box is suspicious imho

@pawel-twardziak
Copy link
Contributor Author

@radix-ng/core @radix-ng/maintainers

Seems like it works like a charm :)

Film.bez.tytulu.mp4
Film.bez.tytulu.1.mp4

@pimenovoleg
Copy link
Contributor

Looks awesome!

Can we make this optional? Something like a feature flag that needs to be enabled (off by default).
Like provideExperimentalZonelessChangeDetection :)

I'm a bit concerned about shadowDOM in MFE and potential side effects.
This is a great solution for improving UX, but I worry not everyone might be ready for it right away :)

@pawel-twardziak what do you think? or enable by default?

@pawel-twardziak
Copy link
Contributor Author

pawel-twardziak commented Dec 22, 2024

Looks awesome!

Can we make this optional? Something like a feature flag that needs to be enabled (off by default). Like provideExperimentalZonelessChangeDetection :)

I'm a bit concerned about shadowDOM in MFE and potential side effects. This is a great solution for improving UX, but I worry not everyone might be ready for it right away :)

@pawel-twardziak what do you think? or enable by default?

Yep true, shadowDom would get some side effects, good point @pimenovoleg 👍
But in general, in context of MFE, that service would be a global and shared one - and the primitives might know nothing about the service then which means the service should not couple itself and the primitive but should be built on top of the promitives so that the primitives don't know about the service.

I will make it internal feature, only for the storybook purposes and will decouple the primitives from the service. The service itself will find and register the prmitives - that would make the service more MFE ready for the future :)

And also will make the service global and experimental, to be provided for playing with it. Documentation will be included.
We will see where it goes. We migth keep and release it or get rid of it in the future :)

UPDATE: it is already an internal feature as I haven't exposed the service yet :) And it is already optional. The service is provided in the storybook stuff. If we don't provide it, nothing will happen because the injection is optional.

@pimenovoleg does it make sense? :)

@pimenovoleg
Copy link
Contributor

Looks awesome!
Can we make this optional? Something like a feature flag that needs to be enabled (off by default). Like provideExperimentalZonelessChangeDetection :)
I'm a bit concerned about shadowDOM in MFE and potential side effects. This is a great solution for improving UX, but I worry not everyone might be ready for it right away :)
@pawel-twardziak what do you think? or enable by default?

Yep true, shadowDom would get some side effects, good point @pimenovoleg 👍 But in general, in context of MFE, that service would be a global and shared one - and the primitives might know nothing about the service then which means the service should not couple itself and the primitive but should be built on top of the promitives so that the primitives don't know about the service.

I will make it internal feature, only for the storybook purposes and will decouple the primitives from the service. The service itself will find and register the prmitives - that would make the service more MFE ready for the future :)

And also will make the service global and experimental, to be provided for playing with it. Documentation will be included. We will see where it goes. We migth keep and release it or get rid of it in the future :)

UPDATE: it is already an internal feature as I haven't exposed the service yet :) And it is already optional. The service is provided in the storybook stuff. If we don't provide it, nothing will happen because the injection is optional.

@pimenovoleg does it make sense? :)

Thanks for the detailed explanation, that makes a lot of sense! 👍

I really like the approach of keeping the service decoupled from the primitives and making it global and experimental for now. It sounds like a solid way to test it without introducing too many risks upfront, especially in the context of MFE.

The fact that the injection is optional and only used in Storybook for now is great—it minimizes potential side effects while still allowing us to experiment and gather feedback.

Looking forward to seeing how it evolves! Let’s keep it iterative and adjust as we learn more. Thanks for considering all these edge cases! 😊

@pimenovoleg
Copy link
Contributor

@pawel-twardziak
It seems like everything is ready for the current version. Do you have any additional thoughts or suggestions to add?

@pawel-twardziak
Copy link
Contributor Author

pawel-twardziak commented Dec 24, 2024

yes, this version is ready @pimenovoleg 👍 But I want to:

  1. make it global
  2. ready to use with MFE
  3. experimental (providers for both regular SPA and MFE environment)
  4. decouple primitives from the service but build on top of them (there will be yet another dediceted directive to do so)
  5. add docs with some examples of how to play with the service in SPA or MFE (out of the box by using the directive or utilizing the service in a custom way)

So, we can either merge this PR as it is only for stories so far and do the above later on or wait for the merge to do the above.
What are you recommending now? :)

@pawel-twardziak
Copy link
Contributor Author

Ok, let's merge it :)

@pawel-twardziak pawel-twardziak merged commit 4b5345a into radix-ng:main Dec 24, 2024
8 checks passed
@pawel-twardziak pawel-twardziak deleted the feature/popover/add-cdk-events-service branch December 24, 2024 12:22
@pawel-twardziak pawel-twardziak self-assigned this Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants