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

[TASK] Refactor GridTableBuilder #958

Merged
merged 1 commit into from
Sep 4, 2024
Merged

[TASK] Refactor GridTableBuilder #958

merged 1 commit into from
Sep 4, 2024

Conversation

linawolf
Copy link
Contributor

This PR is a preparation to make #955 easier to solve. It should not change functionality

releases: main, 1.0

@linawolf
Copy link
Contributor Author

@jaapio as I mainly refactored your code here, please have a look at this

@linawolf linawolf self-assigned this Mar 23, 2024
@linawolf linawolf requested a review from jaapio March 23, 2024 08:33
@jaapio
Copy link
Member

jaapio commented Mar 26, 2024

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.

@jaapio jaapio force-pushed the task/complex-grid-table branch from 603b9ea to efd6733 Compare September 4, 2024 18:58
@jaapio jaapio merged commit 6736477 into main Sep 4, 2024
40 checks passed
@jaapio jaapio deleted the task/complex-grid-table branch September 4, 2024 19:04
@phpdoc-bot
Copy link

💚 All backports created successfully

Status Branch Result
1.x

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

phpdoc-bot added a commit that referenced this pull request Sep 4, 2024
[1.x] Merge pull request #958 from phpDocumentor/task/complex-grid-table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants