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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Remove duplicate menu button when menu forcefully flipped #5497
Fix: Remove duplicate menu button when menu forcefully flipped #5497
Conversation
src/js/components/Menu/Menu.js
Outdated
@@ -88,6 +88,7 @@ const Menu = forwardRef((props, ref) => { | |||
// when there's not enough space below DropButton. This state | |||
// is modified on /Drop/DropContainer.js. | |||
const [alignControlMirror, setAlignControlMirror] = useState(); | |||
const shrunk = alignControlMirror === align.top; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering about this variable name. Wondering if something like alignTop
would make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would initialAlignTop
be a good name for this since we made this boolean to check if there was an align.top when starting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
This PR looks to solve issue #5445.
What does this PR do?
[Still for review] Introduces a boolean to be added to the conditional rendering check of
controlMirror
to see if the menu has shrunk and is being forced to flip vertically, removing the duplicate menu button.Where should the reviewer start?
shrunk
onMenu.js
yarn storybook
and checkoutMenu/Simple
storyWhat testing has been done on this PR?
Manual testing by observing UI changes on storybook (
Menu/Simple
)How should this be manually tested?
Please follow steps in the
Where should the reviewer start?
section.Any background context you want to provide?
What are the relevant issues?
#5445
Screenshots (if appropriate)
Do the grommet docs need to be updated?
None needed
Should this PR be mentioned in the release notes?
Nope, just a simple bug fix.
Is this change backwards compatible or is it a breaking change?
Backwards compatible 馃憤