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: pass event to popover present method #23813

Closed
4 of 6 tasks
jrstnly opened this issue Aug 24, 2021 · 7 comments
Closed
4 of 6 tasks

feat: pass event to popover present method #23813

jrstnly opened this issue Aug 24, 2021 · 7 comments
Labels
package: core @ionic/core package type: feature request a new feature, enhancement, or improvement

Comments

@jrstnly
Copy link

jrstnly commented Aug 24, 2021

Prequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x

Current Behavior

Because the id attribute is used for triggering ion-popover, only a single element on the page is allowed to trigger the popover.

Expected Behavior

Should be able to trigger ion-popover from multiple buttons. The primary use case I see for this is the collapsible header/buttons feature. When using a collapsible header with a collapsible button, it is required to use two buttons to make the collapsible header work properly. Because the inline popover only allows a single element trigger, you can't show the popover when you're scrolled all the way up.

Steps to Reproduce

Create two buttons and an inline popover. Add the id attribute to both buttons and only one of them will work.

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 6.17.0 (/Users/jr.stanley/.nvm/versions/node/v14.17.5/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 6.0.0-beta.4
@angular-devkit/build-angular : 12.1.4
@angular-devkit/schematics : 12.2.2
@angular/cli : 12.1.4
@ionic/angular-toolkit : 4.0.0

Capacitor:

Capacitor CLI : 3.2.0
@capacitor/android : not installed
@capacitor/core : 3.2.0
@capacitor/ios : not installed

Utility:

cordova-res : not installed globally
native-run : 1.4.0

System:

NodeJS : v14.17.5 (/Users/jr.stanley/.nvm/versions/node/v14.17.5/bin/node)
npm : 7.20.6
OS : macOS Big Sur

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Aug 24, 2021
@jrstnly jrstnly changed the title bug: Cannot trigger inline popover from multiple buttons bug: v6 Cannot trigger inline popover from multiple buttons Aug 24, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the issue. If you need to open a popover from multiple buttons, I recommend using the isOpen property instead. Setting the property to true will open the popover and setting it to false will close it.

The trigger property was only ever designed to be associated with a single element, which is why it expects the ID of an element. IDs are unique and should only appear once on your page.

Can you try using the isOpen property instead and let me know if it resolves the issue?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Aug 25, 2021
@ionitron-bot ionitron-bot bot removed the triage label Aug 25, 2021
@jrstnly
Copy link
Author

jrstnly commented Aug 25, 2021

Using the isOpen property did work but it's more cumbersome than I feel like it should be. I don't actually care about doing anything when the popover is dismissed but from what I understand, I need to implement a didDismiss function in order to set the isOpen property back to false when it's closed via the backdrop. I also need to save the click event to a variable in order to pass it to to the popover so that it shows up where the button is on the page.

If the end goal of inline popovers is to simplify the process of using a popover, perhaps exporting IonPopover and exposing the dismiss() and present() methods so that we can just reference the element and call the methods directly on it may be a good intermediate solution. That way the people that want ultra-simple can just use the trigger property and a single button with an id but if you want a little bit more control without using a full component/controller setup you can just grab a reference to the element.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Aug 25, 2021
@liamdebeasi
Copy link
Contributor

The ion-popover component already has its dismiss and present methods exposed, so those should be functional. My main concern about extending trigger to accept multiple elements is that it makes it harder to debug as opposed to having a single source of truth in terms of what triggers a popover to open/close.

I agree that the goal of inline popovers is to simplify the process of using ion-popover while still providing the controller solution for developers who need more advanced control.

Can you give the dismiss method a shot and let me know if that works for your usecase?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Aug 25, 2021
@ionitron-bot ionitron-bot bot removed the triage label Aug 25, 2021
@jrstnly
Copy link
Author

jrstnly commented Aug 25, 2021

I did try using the present method but apparently, I did something wrong because I just tried it again and it's working now. That's the ideal solution for me, but I still need to be able to send the click event for positioning. Is there a way to pass the click event as an argument to the present function or to set it before calling present without saving it as a variable?

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Aug 25, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the update. I think it would be valuable to be able to pass in the click event to the present method directly. In v5 there was little need for this as developers were restricted to only using the popoverController, but now that developers can interact with ion-popover directly, I think having this option is important.

@liamdebeasi liamdebeasi reopened this Aug 26, 2021
@liamdebeasi liamdebeasi added package: core @ionic/core package type: feature request a new feature, enhancement, or improvement labels Aug 26, 2021
@ionitron-bot ionitron-bot bot removed the triage label Aug 26, 2021
@liamdebeasi liamdebeasi changed the title bug: v6 Cannot trigger inline popover from multiple buttons feat: pass event to popover present method Aug 26, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #23827, and a fix will be available in an upcoming release of the Ionic Framework v6 beta.

This will allow you to pass in a click event to the present method, enabling you to present a popover from multiple trigger buttons.

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 25, 2021

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: feature request a new feature, enhancement, or improvement
Projects
None yet
Development

No branches or pull requests

2 participants