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/2423 info popup state always open #2504

Conversation

salindae25
Copy link
Contributor

@salindae25 salindae25 commented Mar 24, 2022

Issue Number: #2423

PR Details

  1. Remove open={supportMenuOpen} from the Popover element. ( this will allow it to have default behavior)
  2. Added onOpen={handleSupportClick} to the Popover element. ( this will make sure to fire the trackEvent)
  3. Removed all supportMenuOpen state related references.

PR Checklist

  • Tests for the changes have been added
  • npm test doesn't throw any error

IMPORTANT: Please review the CONTRIBUTING.md file for detailed contributing guidelines.

yuval-hazaz
yuval-hazaz previously approved these changes Mar 25, 2022
Copy link
Member

@yuval-hazaz yuval-hazaz left a comment

Choose a reason for hiding this comment

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

@salindae25 congrats for your first contribution with Amplication
Great work

@muhsinkamil
Copy link
Contributor

@salindae25 Can we remove onClick at MenuItem on line 110

@salindae25
Copy link
Contributor Author

@salindae25 Can we remove onClick at MenuItem on line 110

yes, it does not affect the functionality of the popover menu.

@muhsinkamil
Copy link
Contributor

yes, it does not affect the functionality of the popover menu.

@salindae25
I see, can you please remove that.

The reason is, trackEvent is fired twice when the icon is clicked.
First onOpen -> opens tooltip and fires trackEvent
second onClick -> does nothing ( since open already ) and fires trackEvent

cc: @yuval-hazaz

@salindae25
Copy link
Contributor Author

salindae25 commented Mar 27, 2022

yes, it does not affect the functionality of the popover menu.

@salindae25 I see, can you please remove that.

The reason is, trackEvent is fired twice when the icon is clicked. First onOpen -> opens tooltip and fires trackEvent second onClick -> does nothing ( since open already ) and fires trackEvent

cc: @yuval-hazaz

sure i'll remove the click event

remove the menu item click event. since menu open on hover.
@mshidlov mshidlov linked an issue Mar 29, 2022 that may be closed by this pull request
@tupe12334 tupe12334 changed the base branch from master to release/0.12.3 March 29, 2022 12:46
@tupe12334 tupe12334 self-requested a review March 29, 2022 12:55
@mshidlov mshidlov merged commit 98868a6 into amplication:release/0.12.3 Mar 29, 2022
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.

🐛 Bug Report: info popup state is always open after clicking
5 participants