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

Refactor the back end main menu so it becomes a regular Knp menu #1101

Merged
merged 2 commits into from Dec 18, 2019

Conversation

@leofeyer
Copy link
Member

leofeyer commented Dec 16, 2019

This PR refactors the back end main menu so it becomes a regular Knp menu and we can render it via {{ knp_menu_render('be_menu') }}. I have extracted these changes from #860, so the other PR becomes smaller and easier to review.

@leofeyer leofeyer added the feature label Dec 16, 2019
@leofeyer leofeyer added this to the 4.9 milestone Dec 16, 2019
@leofeyer leofeyer requested review from ausi, Toflar, bytehead and aschempp Dec 16, 2019
@leofeyer leofeyer self-assigned this Dec 16, 2019
@leofeyer leofeyer mentioned this pull request Dec 16, 2019
3 of 3 tasks complete
Copy link
Contributor

aschempp left a comment

Am I assuming correctly that this will change the menu markup? Which means (our) menu extensions will break?

* Creates a new root node and dispatches an event to fill it with child nodes.
*/
public function create(): ItemInterface
public function buildMainMenu(): ItemInterface

This comment has been minimized.

Copy link
@aschempp

aschempp Dec 16, 2019

Contributor

is it necessary to rename this method? A factory method is usually called create, and not renaming it would keep BC.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 16, 2019

Author Member

Yes, it is necessary, because the follow-up PR adds a buildHeaderMenu() method.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 16, 2019

Author Member

Also, the class is not a factory.

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 16, 2019

Am I assuming correctly that this will change the menu markup?

No, it does not. All CSS classes and HTML attributes are exactly the same as before.

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Dec 16, 2019

Am I assuming correctly that this will change the menu markup?

No, it does not. All CSS classes and HTML attributes are exactly the same as before.

Really? why change the CSS then? https://github.com/contao/contao/pull/1101/files#diff-26367d82ed19d7e3db897008de584c32L259-L269

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Dec 16, 2019

Ah, you are right. The Knp menu uses a different class to track the levels: .tl_level_1 becomes .menu_level_0 and .tl_level_2 becomes .menu_level_1. But the rest remains the same.

@Toflar
Toflar approved these changes Dec 17, 2019
@leofeyer leofeyer merged commit 57f429e into master Dec 18, 2019
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 89.29% (+0.01%) compared to 6b5bb72
Details
@leofeyer leofeyer deleted the feature/knp-header-menu-1 branch Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.