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(FEC-11520): Multi dropdowns are openable in cvaa overlay #638

Merged
merged 16 commits into from
Oct 14, 2021

Conversation

JonathanTGold
Copy link
Collaborator

@JonathanTGold JonathanTGold commented Sep 11, 2021

Description of the Changes

issue: the stop propagation (on dropdown click event) prevent the close menus logic handler (which trigerd by docoment click listener) to be trigerd.

fix: remove the stop propagation and and move the dropdown opening action to the next event loop so that is would bot be canceled by on close logic
sovles: FEC-11520

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

@yairans yairans requested a review from a team September 12, 2021 18:44
* @returns {void}
* @memberof DropDown
*/
onClick = (e: Event): void => {
e.stopPropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the side effects of removing the stopPropagation? we have additional click handlers that we wouldn't want them to catch the event.
As only drop down is using the menu maybe we can lift the event listener up to dropdown and expose the handler as a callback prop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if you mean to the click outside event i did lift it up and now it is managed from the dropdwon, But that in itself is not enough, the challenge is to distinguish between an event that comes from the current component and the rest and i did it by compering the event timestamps

Copy link
Contributor

Choose a reason for hiding this comment

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

@OrenMe @JonathanTGold we need to remove the stopPropagation for closing the other menus. I think it's safe as it has been added after the UI already had most of the click handlers. of course, we have to verify it after this change.
And I prefer the simple timeout solution rather than this event mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your call @yairans

yairans
yairans previously approved these changes Oct 13, 2021
@JonathanTGold JonathanTGold merged commit 54d1224 into master Oct 14, 2021
@JonathanTGold JonathanTGold deleted the FEC-11520 branch October 14, 2021 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants