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: fix hover dropdown menu does not display single menu item #1150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lightbringer1991
Copy link
Contributor

Fix Hover Dropdown Menu does not display when there is only 1 menu item.

Description

Does this PR introduce a breaking change?

  • Yes
  • No

Manual testing step?

Screenshots (if appropriate):

@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #1150 (a20ce90) into master (2e10850) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1150   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           88        88           
  Lines         1297      1307   +10     
  Branches       322       325    +3     
=========================================
+ Hits          1297      1307   +10     
Impacted Files Coverage Δ
src/components/HoverDropdownMenu/index.jsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e10850...a20ce90. Read the comment docs.

@lightbringer1991 lightbringer1991 force-pushed the fix-hover-popover-1-child branch 3 times, most recently from ccb806f to dab42db Compare May 6, 2021 00:17
triggers={['disabled']}
isOpen={isOpen || mouseInPopover}
title={headerText}
popoverContent={<ul className="list-unstyled">{children}</ul>}
popperRef={setPopperNode}
arrowStyles={arrowPosition === 'none' ? { display: 'none' } : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a modifiers props here as well, in case you want to customize it later

Copy link
Contributor

Choose a reason for hiding this comment

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

you can either expose popoverModifiers or considering pre-defined modifiers like set the width of the popper as the width of the reference

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I just found we have a props named popoverProps in the <TextEllipsis /> so maybe pass popoverProps to the <Popover />

Copy link
Contributor

@devharris7 devharris7 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

3 participants