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

DYN-4833-Group-NodesAlignment #13174

Merged
merged 1 commit into from
Aug 5, 2022
Merged

DYN-4833-Group-NodesAlignment #13174

merged 1 commit into from
Aug 5, 2022

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

Fixing nodes alignment when they are inside a Group
Before this fix all the alignment calculations were considering all the nodes selected then also was considering the Group (AnnotationModel), so when aligning to Left, Right, X-Distribute, Y-Distribute the Group was being moved some pixels so the right, left or over X-axis and Y-axis.
My fix was not considering the Group in the selected items so if we are trying to align all the nodes inside the Group, the group location won't change (just if is necessary in some cases like Left or Right alignment).

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

Fixing nodes alignment when they are inside a Group

Reviewers

@QilongTang

FYIs

@avidit

Before this fix all the  alignment calculations were considering all the nodes selected then also was considering the Group (AnnotationModel), so when aligning to Left, Right, X-Distribute, Y-Distribute the Group was being moved some pixels so the right, left or over X-axis and Y-axis.
My fix was not considering the Group in the selected items so if we are trying to align all the nodes inside the Group, the group location won't change (just if is necessary in some cases like Left or Right alignment).
@QilongTang QilongTang added this to the 2.16.0 milestone Aug 4, 2022
@RobertGlobant20
Copy link
Contributor Author

This is a GIF before mi fix, showing Alignment Right and Left
AfterFix_AlignGroupRight_Left

@RobertGlobant20
Copy link
Contributor Author

RobertGlobant20 commented Aug 4, 2022

This is a GIF before my fix showing alignment X and Y Distribute.
AlignGroupDistributeX_Y

@QilongTang
Copy link
Contributor

QilongTang commented Aug 4, 2022

Thanks! @RobertGlobant20 Please also attach a gif.

I do want to check with @Amoursol if these features meant to align everything in the selection, it seems we found that is quite complex and less feasible at this point

@RobertGlobant20
Copy link
Contributor Author

This is a GIF AFTER mi fix, showing Alignment Right and Left
AfterFix_AlignGroupRight_Left

@RobertGlobant20
Copy link
Contributor Author

This is a GIF AFTER mi fix, showing alignment X and Y Distribute.
AfterFix_AlignGroupDistributeX_Y

@RobertGlobant20
Copy link
Contributor Author

Thanks! @RobertGlobant20 Please also attach a gif.

I do want to check with @Amoursol if these features meant to align everything in the selection, it seems we found that is quite complex and less feasible at this point

@QilongTang also please consider that the main problem was that the group was jumping to left, right or up after the alignment and after this fix this shouldn't be happening.

@Amoursol
Copy link
Contributor

Amoursol commented Aug 4, 2022

@QilongTang @RobertGlobant20 Removing the group from the Selection of what to align is perfect as the group itself should be located based on the contents, not controlling the location :)

It's hard to see the differences in the GIF's... what exactly is the difference? If you have a Group selected as part of the alignment, it is ignored now?

@QilongTang
Copy link
Contributor

@QilongTang @RobertGlobant20 Removing the group from the Selection of what to align is perfect as the group itself should be located based on the contents, not controlling the location :)

It's hard to see the differences in the GIF's... what exactly is the difference? If you have a Group selected as part of the alignment, it is ignored now?

Yes, previously the group will jump ahead. I think that is a bug not mentioned by GG but @RobertGlobant20 decided to fix in addition to the position of nodes.

@QilongTang
Copy link
Contributor

Before merging, I think the gifs looks good. I also want to confirm with you @RobertGlobant20 are those bugs only happening when nodes are inside the groups so group selection become a main factor of the bug?

@RobertGlobant20
Copy link
Contributor Author

@QilongTang @RobertGlobant20 Removing the group from the Selection of what to align is perfect as the group itself should be located based on the contents, not controlling the location :)

It's hard to see the differences in the GIF's... what exactly is the difference? If you have a Group selected as part of the alignment, it is ignored now?

@Amoursol the main idea is when aligning the nodes (by selecting one by one or by selecting the group) the alignment behavior should be the same (the group should not be jumping).

@RobertGlobant20
Copy link
Contributor Author

@QilongTang @RobertGlobant20 Removing the group from the Selection of what to align is perfect as the group itself should be located based on the contents, not controlling the location :)
It's hard to see the differences in the GIF's... what exactly is the difference? If you have a Group selected as part of the alignment, it is ignored now?

Yes, previously the group will jump ahead. I think that is a bug not mentioned by GG but @RobertGlobant20 decided to fix in addition to the position of nodes.

@QilongTang I think the jumping ahead bug is mentioned by GG in the Jira task but is not very clear in the description (I think both issues are related the jumping ahead and the nodes position).

@QilongTang
Copy link
Contributor

@RobertGlobant20 Cool, glad to see this feature works and nodes are not overlapping.

@RobertGlobant20
Copy link
Contributor Author

Before merging, I think the gifs looks good. I also want to confirm with you @RobertGlobant20 are those bugs only happening when nodes are inside the groups so group selection become a main factor of the bug?

@QilongTang yes, the problem is happening only when nodes are inside the group (because it was considering the group in the calculation of the span and spacing of each node), when the nodes are out of the group there is no problem (the calculation methods used are the same).

@QilongTang
Copy link
Contributor

@QilongTang QilongTang merged commit 747504d into DynamoDS:master Aug 5, 2022
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