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

[Popover]feat: 2/3(Complete API) #251

Closed
wants to merge 6 commits into from
Closed

[Popover]feat: 2/3(Complete API) #251

wants to merge 6 commits into from

Conversation

onmax
Copy link
Collaborator

@onmax onmax commented Jul 26, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

TODO

  • If we have a stack of dialogs, one on top of other, we have to make sure to handle events correctly and no trigger all of them at same time

@k11q k11q changed the title [Popover]: Complete API [Popover]feat: 2/3(Complete API) Jul 26, 2023
@onmax
Copy link
Collaborator Author

onmax commented Aug 15, 2023

Sorry @zernonia for bothering.

I am updating the Popover with the DismissableLayer and the FocusScope.

But I am getting the following error in the 4df79d0 commit

image

I would appreciate a help on this. I am trying to follow the code from Menu but I didn't see where the error is.

@zernonia
Copy link
Member

Sorry @zernonia for bothering.

I am updating the Popover with the DismissableLayer and the FocusScope.

But I am getting the following error in the 4df79d0 commit

image

I would appreciate a help on this. I am trying to follow the code from Menu but I didn't see where the error is.

Hey @onmax .. I've commented here yaa.. But not sure if that's the cause...

onmax and others added 2 commits August 16, 2023 17:43
* refactor dialog

* add and fix animation

* add forceMount to test animation with Vue navtive transition

* update alertDialog to use Dialog components

* test ssr

* fix autoFocus on cancel trigger not selecting

* add useEmitsAsProps to components to emit event properly
@onmax
Copy link
Collaborator Author

onmax commented Aug 18, 2023

@zernonia I feel overwhelmed. I have a lot of work to do for my job atm.

I tried to finish this component, but I am lost. I need time to focus and understand the new changes like DismissableLayer and FocusScope.

Before having those components, I was implementing the Popover with composables rather than renderless components.

With this change of paradigm, I need to rethink how to implement for example the disabled flag for the Popover and I have no idea where to start. I have to make multiple changes like that, and I basically don't have the time.

So please, feel free to take over so we can mark this feature as done. Otherwise, I will be working on Radix Vue in 10 days or so. 😄

Thanks

@zernonia
Copy link
Member

zernonia commented Aug 18, 2023

@zernonia I feel overwhelmed. I have a lot of work to do for my job atm.

So please, feel free to take over so we can mark this feature as done. Otherwise, I will be working on Radix Vue in 10 days or so. 😄

Thanks

I sorry to hear that you are overwhelmed at work. Yeah I can help made those changes yaa.. I've decided to go with those 2 renderless component (instead of composables) because they expose emits behaviour that could be prevented using preventDefault(), and it sticks closer to the RadixUI API.

No worry ya.. I can have a look at this 😁

@zernonia
Copy link
Member

Thanks for the PR @onmax . I've created another PR (#312), and ported some of your code there.. I will close this PR for further reference for some composables that you've created 😁

@zernonia zernonia closed this Aug 18, 2023
@onmax onmax deleted the onmax/popover branch January 25, 2024 15:48
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.

2 participants