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] Add pimcore 10 and 11 support for new menu creation #406

Merged
merged 9 commits into from
Feb 1, 2023

Conversation

mattamon
Copy link
Contributor

Changes in this pull request

Resolves #403

Additional info

If new preMenuBuild Event is available use the new methods, if not use the pimcoreReady Event to setup the menu

@mattamon mattamon added this to the v4.0.0 milestone Jan 24, 2023
@mattamon mattamon changed the title 403 adjust navigation creation [Task] Add pimcore 10 and 11 support for new menu creation Jan 24, 2023
@mattamon mattamon assigned mattamon and unassigned mattamon Jan 24, 2023
@kingjia90
Copy link
Contributor

kingjia90 commented Jan 30, 2023

I am wondering if we could futher break down and extract menu items to avoid duplicate fragments, at the moment initToolbar and initMenu are quite similar.
https://www.diffchecker.com/ItcLvg3w/

What i want to avoid is that whenever one change something, would always have to apply changes to the other method, but at the same time, it mustn't limit 11 potential improvemenents.

@mattamon
Copy link
Contributor Author

I'm going to take a look into that. I think the main part is how the menu is added, so I could basically make proper functions to get the menus and then in initToolbar add them the old way and in initMenu, use the new approach.

@mattamon
Copy link
Contributor Author

I restructured it like I said above. It is still not the nicest way, but much better than before. I think changing it more, would only make it more complicated.

@kingjia90
Copy link
Contributor

kingjia90 commented Jan 31, 2023

Looks much better but currently the items in initMenu do not make the most of the priority field (adding this property, shouldn't affect the old approach) and wondering if it would be worth to change from

let somevar = somefunc;
if (somevar){
   array.push(somevar);
}

to just array.push(somefunc); and clean the null values (since they can return null) at the end of the function or just before menu.cmf

@mattamon
Copy link
Contributor Author

mattamon commented Feb 1, 2023

Since it is it's own menu, and we do not set the priority anywhere, the priority gets set automatically. I would change this approach tbh because the result would be the same as before, right?

Just pushing the function seems legit, but we would have a problem with the customerMenu, itself. If there are no changes for the submenu, then pushing the function is okay, but I would rather go for the a consistent approach. Wdyt?

@kingjia90

This comment was marked as resolved.

@kingjia90 kingjia90 merged commit ca17b99 into 4.x Feb 1, 2023
@kingjia90 kingjia90 deleted the 403-adjust-navigation-creation branch February 1, 2023 14:15
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.

[Improvement]: Adjust navigation creation
2 participants