Skip to content
This repository has been archived by the owner on Mar 26, 2019. It is now read-only.

Fix panel's close button event listener being assigned multiple times #154

Merged
merged 4 commits into from
Oct 11, 2016

Conversation

leddie24
Copy link
Collaborator

Fix closeButton's event listener from being assigned multiple times. This fixes bug #147. Every time the component is instantiated, the closeButton's click handler is assigned, but never removed. Added a closeButtonClone element in _setEvents and added the event listener to that element instead.

@msftclas
Copy link

Hi @leddie24, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@leddie24 leddie24 changed the title Fix close button event listener being assigned multiple times Fix panel's close button event listener being assigned multiple times Oct 10, 2016
if (closeButton !== null) {
closeButton.addEventListener("click", () => {
let closeButtonClone = closeButton.cloneNode(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just remove the listener if it's there? Does this clone the element several times?

@ericthompson
Copy link
Contributor

ericthompson commented Oct 11, 2016

I'm not seeing any errors in the console 👍

Interesting bug... I'm not sure if this was there before, but after opening a panel, the vertical scrollbar disappears preventing me from scrolling up or down the page (to see the other examples, for instance).

This was in Chrome.

@leddie24
Copy link
Collaborator Author

Hey @ericthompson, we're using the overlay component in conjunction with Panel. Overlay prevents scrolling and user interaction with the page, so it's intended, unless we want to change how Overlay works.

@ericthompson
Copy link
Contributor

ericthompson commented Oct 11, 2016

Yep, but it's leaving the page in a permanently broken state after dismissing the panel. Playing with it a bit more, it only occurs if you dismiss via the X - not via light dismiss by clicking on the Overlay itself. It seems like the Overlay might be staying on the page?

@leddie24
Copy link
Collaborator Author

Ahh, I see. Didn't notice that, I'll check it out.

@leddie24
Copy link
Collaborator Author

All set, PanelHost wasn't hiding Overlay on dismiss.

@gokunymbus
Copy link
Contributor

gokunymbus commented Oct 11, 2016

Approved

Approved with PullApprove

@ericthompson
Copy link
Contributor

ericthompson commented Oct 11, 2016

Approved!

Approved with PullApprove

@leddie24 leddie24 merged commit 4bb084c into master Oct 11, 2016
@leddie24 leddie24 mentioned this pull request Oct 11, 2016
@leddie24 leddie24 deleted the leddie24/panel-dismiss-bug branch October 11, 2016 17:35
@mikewheaton mikewheaton added this to the 1.2.0 milestone Oct 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants