Refactor types and codestyle #7
Conversation
| return []; | ||
| return [ | ||
| 'HTMLAttributes' => [], | ||
| ]; |
There was a problem hiding this comment.
In theory, those options depend on the extension. Currently all nodes and marks support HTMLAttributes, but Heading also has levels for example.
There was a problem hiding this comment.
I moved this to the abstract class, as all of the classes extending Mark have the array like that. The Heading class overrides the method and therefore adds its own implementation. In case one of the marks or nodes doesn't need the HTMLAttributes, you can simply override it with return [];
| public function addOptions() | ||
| { | ||
| return [ | ||
| 'HTMLAttributes' => [], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Can’t we keep that? Technically, it’s not really required (yet), but I tried a lot to keep the API similar to the JavaScript API and it’s required there.
There was a problem hiding this comment.
This is inherited by the Mark abstract class and would be redundant if every class that extends it, implements the same code
| { | ||
| return []; | ||
| } | ||
| abstract public function renderHTML($mark, array $HTMLAttributes = []): array; |
There was a problem hiding this comment.
@hanspagel the $mark parameter and $node parameter on the Node class is unused. Did you have think of any future usecase, where you need to access those values inside the renderHTML method? It's probably for own implementations. Am I write about this?
wip wip
| @@ -12,6 +12,7 @@ | |||
| <ignoreFiles> | |||
| <directory name="vendor"/> | |||
| <file name="src/NewDOMSerializer.php" /> | |||
| <file name="src/DOMSerializerPointer.php" /> | |||
There was a problem hiding this comment.
@hanspagel I think the two files can be completely removed? Do they serve some future ideas?
| @@ -9,7 +9,7 @@ jobs: | |||
| fail-fast: true | |||
| matrix: | |||
| os: [ubuntu-latest, windows-latest] | |||
| php: [8.0] | |||
| php: [7.4, '8.0', 8.1] | |||
There was a problem hiding this comment.
Added PHP 7.4 support for people still stuck to older versions. I'm not 100% sure (didn't test it) but I think you can even install it on 7.3 but I would not suggest to add it as its already out of the maintainance and security support.
Others can use e.g. RectorPHP to generate a <7.4 compatible version of the library
|
Thanks! There are multiple things I do like about the PR, but also some things I don’t like about the PR. One thing I’m hesitant is to change the style of how the nodes and marks are created, because it’s by design that they are plain translations from JavaScript to PHP, even if I’d maybe do some things different if it would only be a PHP package. I’d suggest that I go through the changes and incorporate what I like, and we’ll talk about the rest then. What do you think? |
|
I guess I'll revert 80497b1 so it's easier to see my changes. The code optimization will be put in a separate PR |
|
Sounds good! Wait a little bit though. I have a pending branch I somewhow failed to push. It includes a few core changes. |
This updates the codebase to use types where appropriate and adds PHP 7.4 support.
Changes:
Marks andNodesOpen: