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

Add hook actionMainMenuModifier #70

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

bibips
Copy link

@bibips bibips commented Jun 5, 2023

Questions Answers
Description? Allow main menu to be extended by other modules thanks a new hook actionMainMenuModifier.
Thanks this hook it's possible to have menu items specific per customer by example.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? No issue
How to test? Register hook actionMainMenuModifier and add or edit something into main menu

Hook usage :

    public function hookActionMainMenuModifier(array $hookParams)
    {
          $hookParams['menu']['children'][] = [
              'type' => 'link',
              'label' => $this->trans('Test', [], 'Modules.Test.Menu'),
              'url' => $this->context->link->getModuleLink($this->name, 'pickup'),
              'depth' => 1,
              'children' => [],
              'open_in_new_window' => "0",
              'page_identifier' => $this->trans('Test', [], 'Modules.Test.Menu'),
              'current' => false
          ];
    }

@bibips bibips force-pushed the add-hook-displaymainmenu branch 2 times, most recently from 8d39ed6 to e2c52e5 Compare June 7, 2023 12:58
M0rgan01
M0rgan01 previously approved these changes Jun 19, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Jun 19, 2023

Wouldn't it be better if you could change the $menu variable instead? With this solution, you can display things only after the list, but cannot modify the items.

@bibips
Copy link
Author

bibips commented Jun 19, 2023

Wouldn't it be better if you could change the $menu variable instead? With this solution, you can display things only after the list, but cannot modify the items.

I like this Idea

@bibips bibips force-pushed the add-hook-displaymainmenu branch from e2c52e5 to b0601c8 Compare June 19, 2023 11:35
@bibips bibips changed the title Add hook displayMainMenu Add hook actionMainMenuModifier Jun 19, 2023
@bibips bibips requested a review from M0rgan01 July 11, 2023 11:52
@bibips
Copy link
Author

bibips commented Oct 16, 2023

@Hlavtox @M0rgan01 any updates for this PR ?

@M0rgan01
Copy link

Sorry, I lost the thread! Yes it's ok for me :) You will need to add the new hook on the core side (in hook.xml file)

M0rgan01
M0rgan01 previously approved these changes Oct 16, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Oct 16, 2023

@M0rgan01 I think we shouldn't, the core should include no data or references to any modules I think.

@M0rgan01
Copy link

I thought that was the case :) Everything is good

@AureRita AureRita self-assigned this Oct 18, 2023
@AureRita
Copy link

AureRita commented Oct 18, 2023

Hi @bibips

Thank you for your PR, I tested it and unfortunately this one doesn't works as expected as you can see :

image

Indeed, the "use Hook;" doesn't seems to works and, because of that, the hook isn't accessible

image

Thank you

Thanks this hook it's possible to alter main menu. For instance you can
display menu items per customer or per group of customer
@bibips
Copy link
Author

bibips commented May 31, 2024

@AureRita Excuse me for the delay, I've removed use Hook; as suggested

@jtraulle
Copy link

This would be a great addition for external modules to be able to add links to the main menu.
Anything missing here @AureRita for this to be merged ?

Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

I don't see much value in this… isn't the sole purpose of menu that it's purely configurable by people?

Do you have any real use case for this hook?

@bibips
Copy link
Author

bibips commented Nov 18, 2024

I don't see much value in this… isn't the sole purpose of menu that it's purely configurable by people?

Do you have any real use case for this hook?

Hi @Hlavtox,

I've created this hook to add or remove menu items according some criterias. By instance in my shop I have a menu item only available for a group of customer. Thanks to this hook, we can check if the customer is logged in or if the customer matches certain criteria (such as country, device type, customer configuration, etc...) before making a menu item available.

Without this hook, we can't.

@bibips
Copy link
Author

bibips commented Feb 21, 2025

Hi @Hlavtox,

Does this use case convince you of the hook's usefulness ?

Copy link

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @bibips

@jolelievre jolelievre dismissed Hlavtox’s stale review March 3, 2025 09:40

The explained use case makes sense to me and there are probably many others. The implementation is also ok so I dismiss your blocker for now @Hlavtox.
If you are other worries don't hesitate to re-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

None yet

8 participants