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(context menu): Fix for context menu use when zoomed out #249

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

jeff-phillips-18
Copy link
Member

What

Closes #247

Description

Adds the option to not raise the label when the node is hovered. This keeps the label on the same layer as the node which avoids getting a mouse leave event when moving from the node to the label. For applications that are raising the node to the TOP_LEVEL on hover, they should pass raiseLabelOnHover={false} to the DefaultNode.

Also, when the context menu is open, treat the node label the same as if hovering so that it remains visible and un-truncated while the context menu is open.

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Screen shots / Gifs for design review

Hover_to_context

@patternfly-build
Copy link

patternfly-build commented Nov 6, 2024

Copy link
Collaborator

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Awesome work on the context menu fix, @jeff-phillips-18 ! Looks great. Wondering if it might be worth applying this fix to the Styles -> Labels demos as well, just to keep the hover behavior consistent across the board. What are your thoughts?

@@ -118,6 +118,8 @@ interface DefaultNodeProps {
onContextMenu?: (e: React.MouseEvent) => void;
/** Flag indicating that the context menu for the node is currently open */
contextMenuOpen?: boolean;
/** Flag indicating the label should move to the top layer when the node is hovered */
raiseLabelOnHover?: boolean;
Copy link
Collaborator

@jenny-s51 jenny-s51 Nov 6, 2024

Choose a reason for hiding this comment

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

Could we add a TODO here as a reminder to remove this in the next breaking change release. WDYT @jeff-phillips-18 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could change it to default to false but we need to know if the application is already putting the node on the TOP_LAYER or not.

Copy link
Collaborator

@jenny-s51 jenny-s51 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 making these fixes @jeff-phillips-18 , hover works great in the updated demos 🎉

Just noticed when the context menu is accessed / clicked from the Context Menu Labels demo , the node icon is no longer visible :
Screenshot 2024-11-06 at 2 03 49 PM

@jeff-phillips-18
Copy link
Member Author

Just noticed when the context menu is accessed / clicked from the Context Menu Labels demo , the node icon is no longer visible

@jenny-s51 Fixed. Thanks.

Copy link
Collaborator

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Thanks @jeff-phillips-18 , latest changes look great! LGTM 🚀

@jenny-s51 jenny-s51 merged commit 7260458 into patternfly:v5 Nov 11, 2024
8 checks passed
@jeff-phillips-18 jeff-phillips-18 deleted the context-menu-hover branch November 11, 2024 18:34
@jeff-phillips-18
Copy link
Member Author

Released in v5.4.1

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.

3 participants