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

Improve Group shape to allow moving all contained shapes at once. #690

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

DennisBirkholz
Copy link
Contributor

This pull requests modifies the Group container shape so moving it around moves all contained shapes with it.
To make this work reliably all offsets, extents and dimensions are calculated based on the contained shapes and not cached anymore.
I also fixed the Group writing part in the PowerPoint2007 writer which used the extentX/Y methods to get the width and height. (The XML nodes/attributes are named really misleading here)

Thank you for considering this contribution and thank you for your work!

@auto-assign auto-assign bot requested a review from Progi1984 December 8, 2021 17:11
@DennisBirkholz DennisBirkholz changed the title Improve Group shape to allow moving all contained shapes at one. Improve Group shape to allow moving all contained shapes at once. Dec 8, 2021
@Progi1984 Progi1984 changed the base branch from develop to master September 1, 2024 10:53
@Progi1984
Copy link
Member

@DennisBirkholz Could you rebase your PR, and update the changelog and if you Can add some unit tests, it would be perfect ?

After that (and a review), I will merge the PR.

@DennisBirkholz
Copy link
Contributor Author

DennisBirkholz commented Sep 8, 2024

He @Progi1984, I will rebase in the upcoming week and update the changelog and I will try to come up with some meaningful test cases.

@Progi1984
Copy link
Member

Thanks @DennisBirkholz

@DennisBirkholz DennisBirkholz force-pushed the pull-group-shape branch 2 times, most recently from 079c5f3 to e4c7c4f Compare September 10, 2024 12:38
@DennisBirkholz
Copy link
Contributor Author

@Progi1984: I rebased my pull request, added a change log entry and adjusted the tests for the Group shape (I don't think additional tests are required). If there is anything else I should change, please point it out, thanks.

The failing PHPUnit-Test is unrelated to my Pull request. PptChartsTest.php#L778 causes the test failure in case the random call returns 0.

@@ -5,6 +5,7 @@
## Enhancements

- `phpoffice/phpspreadsheet`: Allow version 1.9 or 2.0 by [@Progi1984](https://github.com/Progi1984) fixing [#790](https://github.com/PHPOffice/PHPPresentation/pull/790), [#812](https://github.com/PHPOffice/PHPPresentation/pull/812) in [#816](https://github.com/PHPOffice/PHPPresentation/pull/816)
- Group Shape: moving the shape now moves all contained shapes. Offsets and size are calculated based on the contained shapes [#690](https://github.com/PHPOffice/PHPPresentation/pull/690)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Group Shape: moving the shape now moves all contained shapes. Offsets and size are calculated based on the contained shapes [#690](https://github.com/PHPOffice/PHPPresentation/pull/690)
- Group Shape: moving the shape now moves all contained shapes. Offsets and size are calculated based on the contained shapes by [@DennisBirkholz](https://github.com/DennisBirkholz) in [#690](https://github.com/PHPOffice/PHPPresentation/pull/690)

Copy link
Member

Choose a reason for hiding this comment

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

@DennisBirkholz a small feedback

Copy link
Member

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

A small feedback in changelog

OffsetX/Y, ExtentX/Y, Width and Height are always calculated to prevent desync between Group and contained shapes.
@DennisBirkholz
Copy link
Contributor Author

@Progi1984 Changelog is updated according to your feedback

@Progi1984 Progi1984 merged commit 04700ed into PHPOffice:master Sep 12, 2024
28 checks passed
@Progi1984
Copy link
Member

@DennisBirkholz Thank you for your contribution 🎇

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

Successfully merging this pull request may close these issues.

2 participants