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(menu): Don't render Menu when it's closed #89

Merged
merged 1 commit into from Aug 2, 2018

Conversation

ryanseddon
Copy link
Contributor

Description

If you tried to make dynamic prop changes to Menu it would throw due to Popper.js trying to get the computed styles of null.

Detail

Simple fix here is to just not render the Menu when the menu is closed.

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 330

  • 21 of 23 (91.3%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 96.143%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/menus/src/containers/MenuContainer.js 21 23 91.3%
Totals Coverage Status
Change from base Build 327: -0.003%
Covered Lines: 1683
Relevant Lines: 1697

πŸ’› - Coveralls


if (newSelectedKey === undefined) {
newSelectedKey = selectedKey;
{isOpen && (
Copy link

Choose a reason for hiding this comment

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

I believe this is the only change? πŸ˜‚

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep the rest was prettier reformatting, best viewed with ?w=1

Copy link

Choose a reason for hiding this comment

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

πŸ’― thanks, Ryan~

if (!isOpen) {
return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was deleted too @zillding

@ryanseddon
Copy link
Contributor Author

@jzempel coveralls has got confused due to the prettier format its complaining about new stuff that isn't actually new

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Thanks, Ryan! We can merge around the coveralls check.

@ryanseddon ryanseddon merged commit f79518b into master Aug 2, 2018
@ryanseddon ryanseddon deleted the ryan/menu-closed-fix branch August 2, 2018 22:44
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

4 participants