verbb / navigation

A Craft CMS plugin to create navigation menus for your site.
Other
90 stars 22 forks source link

When "Max nodes per level" reached no node move possible #373

Closed d--j closed 10 months ago

d--j commented 10 months ago

Describe the bug

After #359 you cannot change the node order when you reached the maximum amount of nodes on one level.

Here

https://github.com/verbb/navigation/blob/044e776ec184db3a64cce38405819ae32feafd2d/src/elements/Node.php#L302-L320

you only need the call to setTempNodes when the node is not already on the same level (or skip the check in that case all-together).

Side note: Adding the event handler in the init method of the Nav element will add an event handler for every instance of a Nav element. This is kinda strange and possibly very inefficient. I suggest moving it to https://github.com/verbb/navigation/blob/044e776ec184db3a64cce38405819ae32feafd2d/src/Navigation.php#L167

Steps to reproduce

  1. Create a navigation with Setting "Max Nodes per Level" "Level 1" set to 2
  2. Create two navigation nodes
  3. Try to move the second navigation node to the top

Craft CMS version

Craft Pro 4.5.9

Plugin version

2.0.22

Multi-site?

Yes

Additional context

No response

engram-design commented 10 months ago

Good call on the plugin-level event listener. I'll admit, I'm not quite sure what I was thinking there at the time.

Should be fixed for the next release. To get this early, run composer require verbb/navigation:"dev-craft-4 as 2.0.21".

d--j commented 10 months ago

@engram-design thanks, this fix is working 👍