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

Add support for style objects to the element constructor #2362

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hazington
Copy link
Contributor

@hazington hazington commented Dec 30, 2022

Description

The current version does not support passing style objects to the constructor of an element. Only arrays are supported. But for a full OOP approach the element constructor should also or only support style objects. As the setNewStyle method is only used internally, the returnObject param can be ignored when passing a style element instance as styleValue via constructor.

Fixes # (issue)

Elements do not support style objects via constructor.

Checklist:

  • I have run composer run-script check --timeout=0 and no errors were reported
  • The new code is covered by unit tests (check build/coverage for coverage report)
  • I have updated the documentation to describe the changes

The current version does not support passing style objects to the constructor of an element. Only arrays are supported. But for a full OOP approach the element constructor should also support style objects. As the setNewStyle method is only used internally, the returnObject param can be ignored when passing a style element instance as styleValue via constructor.
@hazington
Copy link
Contributor Author

I'll cover this one with unit tests until the end of this week.

@hazington
Copy link
Contributor Author

@Progi1984

UnitTests added, documentation modified. You're welcome!

…cell to actually cover the new style object support via constructor.
@hazington
Copy link
Contributor Author

While I was working on this issue I realized that the style behavior should be revised. The constructor and set style methods should only support the actual style object. The array support should be abandoned. Instead a static fromArray method should be added to each style.

Before:

$cell = new cell(100, [
   // some style
]);

After:

$cell = new cell(100, CellStyle::fromArray([
   // some style
]);

Since this would be a breaking change, I decided to go for the workaround committed here to at least add support for style objects for elements like cell. I've also noticed that the style behavior is quite different across the various element types. The text object for example always uses the same object passed via constructor. Other elements prefer cloning.

I think that we should also add a merge method to element styles and a dedicated className array. All elements should always return a style object via getStyle() method.

[...]
public function addClassName(string $className): void;
public function hasClassName(string $className): bool;
public function clearClassNames(): void;
public function hasClassNames(): bool;
public function getClassNames(): array;
public function getStyle(): <StyleObject>;
[...]

With the merge method something like this could happen in the renderer:

$style = $element->getStyle();

foreach ($phpWord->getStyles($element->getStyleNames() as $classStyle) {
    if ($classStyle::class === $style::class) {
       $style->merge($classStyle);
   }
}

This would allow adding multiple class names to an object but also overwrite specific attributes it. My private html2word library does support multiple classes in a similar way, but I don't know how Word files are actually rendered. If Word only supports one class name per element, then passing multiple classes to an element doesn't make sense. But even then, the set style method should only support `string|'.

@Progi1984 Progi1984 force-pushed the master branch 3 times, most recently from 2d9f999 to e458249 Compare August 30, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant