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

feat(ContextMenu): Intelligently position context menu #3119

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

m7kvqbe1
Copy link
Member

@m7kvqbe1 m7kvqbe1 commented Feb 22, 2022

Related issue

Closes #2465

Overview

Intelligently position the ContextMenu based on the available screen real-estate.

Link to preview

https://6214cbcefa42c7b48aad1817--optimistic-williams-bf676e.netlify.app

Reason

Remove the need for the consumer to be explicit about the positioning of the menu.

Work carried out

  • Rename useHideShow hook isOpen param to initialIsOpen
  • Improve the behaviour of the useClickMenu hook and leverage useFloatingElement

@m7kvqbe1 m7kvqbe1 added Type: Enhancement New feature or request Package: react-component-library Package/code type Release: Next major Release version Type: Development Related to a design system component labels Feb 22, 2022
@m7kvqbe1 m7kvqbe1 self-assigned this Feb 22, 2022
@m7kvqbe1 m7kvqbe1 force-pushed the feat/intelligently-position-context-menu branch 3 times, most recently from fcfd026 to 0d10733 Compare February 22, 2022 12:03
Copy link
Collaborator

@jpveooys jpveooys left a comment

Choose a reason for hiding this comment

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

Works well from testing 👍

There's a build error in the smoke test fyi.

Also, I'm not sure about initialIsOpen in all of these cases – some of them try to react to the prop changing as well I think (there's a useEffect in useOpenClose).

packages/react-component-library/src/hooks/useClickMenu.ts Outdated Show resolved Hide resolved
@m7kvqbe1 m7kvqbe1 force-pushed the feat/intelligently-position-context-menu branch from 9accadb to 95247e0 Compare February 23, 2022 10:47
Copy link
Collaborator

@jpveooys jpveooys left a comment

Choose a reason for hiding this comment

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

I still have this question:

Also, I'm not sure about initialIsOpen in all of these cases – some of them try to react to the prop changing as well I think (there's a useEffect in useOpenClose).

Otherwise looks good 👍

@m7kvqbe1 m7kvqbe1 force-pushed the feat/intelligently-position-context-menu branch from 95247e0 to abfb7d3 Compare February 28, 2022 09:09
@m7kvqbe1
Copy link
Member Author

m7kvqbe1 commented Feb 28, 2022

Dropped the initialIsOpen change as the useEffect hook means it's in most cases reacting to the prop change.

@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

88.5% 88.5% Coverage
0.0% 0.0% Duplication

@m7kvqbe1 m7kvqbe1 merged commit 7893e1f into release/3.0.0 Feb 28, 2022
@m7kvqbe1 m7kvqbe1 deleted the feat/intelligently-position-context-menu branch February 28, 2022 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: react-component-library Package/code type Release: Next major Release version Type: Development Related to a design system component Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants