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

added property to set whether the outline is hull or rect #114

Merged
merged 7 commits into from
Nov 14, 2023

Conversation

leandroberetta
Copy link
Collaborator

@leandroberetta leandroberetta commented Oct 24, 2023

Closes #113

Description

A new property in the DefaultGroup was added to set whether the outline of the group is hulled or rect.

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

@jeff-phillips-18 jeff-phillips-18 marked this pull request as ready for review October 25, 2023 13:52
@jeff-phillips-18
Copy link
Member

@jenny-s51 @nicolethoen Could you PTAL?

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.

Thank you for your contribution @leandroberetta ! Visually this is looking good - just a couple of comments:

It looks like when shift+clicking+dragging a node, the default behavior should move it out of a group. When this new prop is added, I can no longer seem to move the node out of the group. Is this to be expected?

Nit: I'm also wondering since the hulled outline was already the default and these changes introduce the rectangular outline, should we rename it to something like rectOutline instead? I think semantically this makes more sense and we could just set rectOutline instead of setting manually the prop value to false, however this isn't a blocker.

cc @jeff-phillips-18 what do you think?

@jeff-phillips-18
Copy link
Member

Nit: I'm also wondering since the hulled outline was already the default and these changes introduce the rectangular outline, should we rename it to something like rectOutline instead? I think semantically this makes more sense and we could just set rectOutline instead of setting manually the prop value to false, however this isn't a blocker.

I'm in favor of the hulledOutline just sounds better to me even if it is the default but I could go either way.

jeff-phillips-18 and others added 2 commits November 10, 2023 09:13
Fix for dragging out of rect group, adds option to Node Options for rectangle groups
@leandroberetta
Copy link
Collaborator Author

Thank you for your contribution @leandroberetta ! Visually this is looking good - just a couple of comments:

It looks like when shift+clicking+dragging a node, the default behavior should move it out of a group. When this new prop is added, I can no longer seem to move the node out of the group. Is this to be expected?

Nit: I'm also wondering since the hulled outline was already the default and these changes introduce the rectangular outline, should we rename it to something like rectOutline instead? I think semantically this makes more sense and we could just set rectOutline instead of setting manually the prop value to false, however this isn't a blocker.

cc @jeff-phillips-18 what do you think?

@jenny-s51, thanks for the review, @jeff-phillips-18 helped me with the issue you reported.

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.

Thank you @leandroberetta @jeff-phillips-18 latest changes LGTM 🚀

@jeff-phillips-18 jeff-phillips-18 merged commit a8a63ce into patternfly:main Nov 14, 2023
5 checks passed
Copy link

🎉 This PR is included in version 5.2.0-prerelease.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to DefaultGroup to support rectangular shape
3 participants