Skip to content

Conversation

@ryanseddon
Copy link
Contributor

Description

On menu unmount we had some code to remove the document event listener to support the click outside functionality.

Detail

We're in fact adding another listener again on unmount rather than removing it.

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 💅 view component styling is based on a Garden CSS
    component
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 💂‍♂️ includes new unit and snapshot tests
  • 📒 any new files are included in the packages src/index.js export
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

Copy link

@seangoedecke seangoedecke left a comment

Choose a reason for hiding this comment

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

🙇

@coveralls
Copy link

coveralls commented Aug 21, 2018

Pull Request Test Coverage Report for Build 501

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 96.089%

Totals Coverage Status
Change from base Build 495: -0.08%
Covered Lines: 1692
Relevant Lines: 1706

💛 - Coveralls

Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@ryanseddon ryanseddon force-pushed the ryan/menu_remove_event_listener branch from fb1c9a6 to 495544e Compare August 22, 2018 04:05
Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

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

I'm not sure how this decreased the number of tested lines since this is actually testing the desired action.

I think this one should be ok with pushing through.

@ryanseddon ryanseddon merged commit d8e5bb1 into master Aug 23, 2018
@ryanseddon ryanseddon deleted the ryan/menu_remove_event_listener branch August 23, 2018 01:22
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.

6 participants