Skip to content

!!![TASK] Restructure Menu handling #799

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Jan 5, 2024
Merged

!!![TASK] Restructure Menu handling #799

merged 15 commits into from
Jan 5, 2024

Conversation

linawolf
Copy link
Contributor

@linawolf linawolf commented Jan 1, 2024 •
edited
Loading

Depends on #798

@linawolf linawolf marked this pull request as draft January 1, 2024 18:21
@linawolf linawolf self-assigned this Jan 1, 2024
@linawolf linawolf force-pushed the task/improve-menu-items2 branch from f6d5736 to 1a6cc43 Compare January 2, 2024 02:46
@linawolf linawolf requested a review from jaapio January 2, 2024 02:47
@linawolf linawolf marked this pull request as ready for review January 2, 2024 02:47
@linawolf linawolf force-pushed the task/improve-menu-items2 branch from 1a6cc43 to 9133d26 Compare January 2, 2024 02:48
@linawolf
Copy link
Contributor Author

linawolf commented Jan 2, 2024

@wouterj does this also solve your issue with toctrees with maxdepth without titlesonly?

@linawolf linawolf force-pushed the task/improve-menu-items2 branch from 6cda9a6 to 0e654c1 Compare January 2, 2024 17:01
@linawolf linawolf mentioned this pull request Jan 2, 2024
Closed
@linawolf linawolf force-pushed the task/improve-menu-items2 branch from 0e654c1 to 4211db1 Compare January 2, 2024 19:52
jaapio
jaapio requested changes Jan 2, 2024
View reviewed changes
Copy link
Member

@jaapio jaapio left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this massive improvement. I added a number of comments to improve the code a bit. In general this looks very good!

...ides/src/Compiler/NodeTransformers/MenuNodeTransformers/AbstractMenuEntryNodeTransformer.php Outdated
Comment on lines 155 to 156
$documentEntryInToc->setParent($compilerContext->getDocumentNode()->getDocumentEntry());
$compilerContext->getDocumentNode()->getDocumentEntry()->addChild($documentEntryInToc);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move the addChild call to the setParent method in the DocumentEntry class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

document entries know nothing about the document node

linawolf and others added 12 commits January 3, 2024 12:12
@linawolf @jaapio
Update packages/guides/src/Compiler/NodeTransformers/MenuNodeTransfor…
ffcd7d8 
…mers/AbstractMenuEntryNodeTransformer.php

Co-authored-by: Jaap van Otterdijk <jaap@ijaap.nl>
@linawolf @jaapio
Update packages/guides/src/Compiler/NodeTransformers/MenuNodeTransfor…
c850c52 
…mers/AbstractMenuEntryNodeTransformer.php

Co-authored-by: Jaap van Otterdijk <jaap@ijaap.nl>
@linawolf @jaapio
Update packages/guides/src/Compiler/NodeTransformers/MenuNodeTransfor…
adaf7e0 
…mers/AbstractMenuEntryNodeTransformer.php

Co-authored-by: Jaap van Otterdijk <jaap@ijaap.nl>
@linawolf @jaapio
Update packages/guides/src/Compiler/NodeTransformers/MenuNodeTransfor…
9b15f81 
…mers/AbstractMenuEntryNodeTransformer.php

Co-authored-by: Jaap van Otterdijk <jaap@ijaap.nl>
@linawolf
[TASK] Refactor isEqualAbsolutePath and isEqualRelativePath into matches
f26861f
@linawolf
[TASK] Refactor isEqualAbsolutePath and isEqualRelativePath into matches
32dd079
@linawolf
[TASK] Move shared functions to trait
2904b83
@linawolf
[TASK] Remove unused constructors
68b4aee
@linawolf
[TASK] Remove unused constructors
0a9922a
@linawolf
[TASK] Remove superfluous constructor param
d1e6385
@linawolf
[TASK] Make node transformers stateless
818bac3
@linawolf
[TASK] Make helper methods static
456cce2
@linawolf
Copy link
Contributor Author

linawolf commented Jan 4, 2024

@jaapio, there are a few open conversation where I asked for explanaitions, the rest is finished

@linawolf linawolf requested a review from jaapio January 4, 2024 12:12
@linawolf linawolf mentioned this pull request Jan 4, 2024
Closed
@wouterj
Copy link
Contributor

wouterj commented Jan 4, 2024 •
edited
Loading

The results look great, thanks for this effort Lina! Just tested this PR in my test repository and the maxdepth seems 100% correct now!

I've found one minor error since this PR: the MenuEntryNode::level property seems weirdly inconsistent now.
When rendering the main menu using {{ renderMenu('mainMenu') }} in a template, the first level (document titles at the root) appear to have the level property set to 2 instead of 1.
When rendering a toctree using the directive, it seems like the first level has the property set to 0 instead of 1 (I'm not sure if this always used to be the case).

@jaapio jaapio mentioned this pull request Jan 5, 2024
Closed
jaapio
jaapio approved these changes Jan 5, 2024
View reviewed changes
Copy link
Member

@jaapio jaapio left a comment

Choose a reason for hiding this comment

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

This works as is, other comments are picked up in flow up items.

@jaapio jaapio merged commit d160f4c into main Jan 5, 2024
@jaapio jaapio deleted the task/improve-menu-items2 branch January 5, 2024 09:55
linawolf added a commit that referenced this pull request Jan 6, 2024
@linawolf
[BUGFIX] Generate Warning when menu entry was not found
959c35f 
Since #799 an exception was thrown
@linawolf linawolf mentioned this pull request Jan 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaapio jaapio jaapio approved these changes

Assignees

@linawolf linawolf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@linawolf @wouterj @jaapio