[TASK] Refactor GridTableBuilder #958
Conversation
|
@jaapio as I mainly refactored your code here, please have a look at this |
|
Sorry I didn't have the headspace to review this in detail. Gridtables are a real pain. My original idea with the tables was that all tables have the same structure. They do have rows, and columns that exist of cells. Once we have the cells we can use the producers to create the content nodes of the cells. Where they mostly have just InlineNodes, and sometimes some more complex nodes like lists, or directives. To make this work I had the intention to split everything into normal sized cells as there are no rowspans and colspans. And then start merging those in a second iteration. And the last step of parsing would be parsing the cell contents. However I faced some issues in this approach. Colspan content is longer that a column, so words are cut in parts when they stretch over multiple columns, whitespaces are an issue ect. I do not remember exactly how I solve this, and if I did solve it anyway or just moved on with my focus to some other pain point back then. I wouldn't be surprised if there were more issues with the parsing of tables as the original code was even more of a mess than what we have right now. My suggestion of this refactoring is to add loads of tests that cover the table parsing and rendering and start improving things. step by step. We already have quite a few test covering the current behavior but I'm not sure we cover everything as there are way to many options create a table. This is a system on it's own. On the other hand... as long the tests are green, it's ok to proceed the refactoring process. I have no strong opinion on how this should look like. |
releases: main, 1.0
jaapio
force-pushed
the
task/complex-grid-table
branch
from
603b9ea to
efd6733
Compare
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
[1.x] Merge pull request #958 from phpDocumentor/task/complex-grid-table
This PR is a preparation to make #955 easier to solve. It should not change functionality
releases: main, 1.0