Bugfix/13266 Update the processing of items for the top menu #13323

arnoudhgz · 2018-01-23T08:19:04Z

Description

The counter in the foreach loop was not being calculated correctly. This resolved in the 'last' css class not being set on the output when one or more parent items are not active.

With this PR the items will be filtered first before they are being processed for the html output.

Fixed Issues (if relevant)

Topmenu 'last' class not being set if the a parent is inactive #13266: Topmenu 'last' class not being set if the a parent is inactive

Manual testing scenarios

Do a clean install of Magento 2.2 with sample data
Set a level 1 category (e.g. Gear or Gift Cards) inactive
Refresh the appropriate cache
Inspect the menu and see there is a class last on the last active top menu item

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

orlangur · 2018-01-23T12:33:54Z

app/code/Magento/Theme/Block/Html/Topmenu.php

        foreach ($children as $child) {
-            if ($childLevel === 0 && $child->getData('is_parent_active') === false) {
-                continue;
-            }
            $child->setLevel($childLevel);
            $child->setIsFirst($counter == 1);
            $child->setIsLast($counter == $childrenCount);


Approach looks generally good to me but I don't the idea of separate filtering loop.

I believe instead of removing if and introducing filtering we need just to track last item properly.

For example, remove $childrenCount variable, declare $lastChild = null before loop, do

$child->setIsLast(false); $lastChild = $child;

in loop and

if ($lastChild) { $lastChild->setIsLast(true); }

after it.

As always, please squash changes into single commit and force push after changes are performed.

@orlangur thank you for your feedback.

Your approach was also the first thing that I wanted to do, but that does not work because the $html with all the html for the $schildren is generated in the foreach loop.

If you do the setIsLast() after the loop the class last is not added to the last visible menu item (which was the whole point for this commit ;) )

I squashed the commits 👍

Please remove $itemPosition as it seems to be unused

Track $lastChild as requested earlier

Split one loop into two: in first we process children, in second generate actual HTML with concatenation

Feel free to introduce new private methods to get rid of @SuppressWarnings(PHPMD.CyclomaticComplexity) and @SuppressWarnings(PHPMD.NPathComplexity)

arnoudhgz · 2018-01-24T12:21:04Z

@orlangur can you please take a look at my response?

orlangur · 2018-01-24T13:05:39Z

@arnoudhgz yeah-yeah, I saw your response and even started to reply, will search for a tab in my browser :) Sorry for delay and thanks for quicks feedback 👍

arnoudhgz · 2018-02-02T19:30:25Z

Removed the label, the code does not need the update as suggested. There should be taken a closer look in the issue and the proposed solution in this commit.

The suggested change by @orlangur will not help to solve the issue, because the HTML is generated inside the foreach loop.

The suggested change does not solve the issue because the HTML is generated inside the foreach loop

orlangur · 2018-02-03T22:06:17Z

Label should be removed no earlier than it is confirmed by reviewer. Current approach is not acceptable from computational complexity perspective, I was checking the best way to split loop into two, will share exact suggestion soon.

arnoudhgz · 2018-02-03T22:25:01Z

Great! A solution with perfect code that will also solve the bug has my preference too 😉

arnoudhgz · 2018-02-10T15:29:38Z

@orlangur do you have any idea when you can take a look into this?

orlangur · 2018-02-12T14:26:17Z

app/code/Magento/Theme/Block/Html/Topmenu.php

        foreach ($children as $child) {
-            if ($childLevel === 0 && $child->getData('is_parent_active') === false) {
-                continue;
-            }
            $child->setLevel($childLevel);
            $child->setIsFirst($counter == 1);
            $child->setIsLast($counter == $childrenCount);


Please remove $itemPosition as it seems to be unused

Track $lastChild as requested earlier

Split one loop into two: in first we process children, in second generate actual HTML with concatenation

Feel free to introduce new private methods to get rid of @SuppressWarnings(PHPMD.CyclomaticComplexity) and @SuppressWarnings(PHPMD.NPathComplexity)

orlangur · 2018-02-19T14:17:24Z

Hi @arnoudhgz, please check my previous comment :)

The counter in the foreach loop was not being calculated correctly. This resolved in the 'last' css class not being set on the output when one or more parent items are not active. With this commit the items will be filtered first before they are being processed for the html output. Move the filtering to seperate method Moved the filter of children without active parent to a seperate method. This will make the whole method more readable and understandable. Also this is a little optimization; the foreach loop will only be run if the child level = 0. Apply consistent class import for Node

arnoudhgz · 2018-02-23T21:51:14Z

@orlangur the splitting into separate private function would be a good thing to do. But I can't agree with the tracking of the $lastChild, you will have to overwrite the last item in the array again with this variable. I can't with a good conscience loop twice over the array in your suggested way.

In my approach of making the array first smaller you have to loop over less items in the second foreach. This seems cleaner, more efficient.

I don't get the thing about the computational complexity perspective, you mentioned earlier, can you make it more specific what I did wrong compared to that perspective?

Before I start working again on making this function cleaner in more separate private functions can you elaborate more about your suggested approach?

Maybe you can make an example in (pseudo) code...

orlangur · 2018-07-13T17:29:53Z

Hi @arnoudhgz, could you please recreate this branch?

During review I noticed that $children->delete($child); is O(1) in fact thus I was wrong about nonlinear complexity.

The only thing I would ask you to do is rename removeOrphans to something like removeChildrenWithoutActiveParent - to not introduce a new term "orphan".

I lost your PR somewhere among hundreds of open browser tabs, sorry about that.

arnoudhgz · 2018-07-13T22:37:44Z

@orlangur yes, no problem. I will add it again.

Should I add the declare(strict_types=1); to the PHP file, or is that only for the 2.3-develop branch?

orlangur · 2018-07-15T11:05:35Z

@arnoudhgz you can but it's mandatory for newly added files only.

orlangur self-assigned this Jan 23, 2018

orlangur previously requested changes Jan 23, 2018

View reviewed changes

orlangur added the Progress: needs update label Jan 23, 2018

arnoudhgz force-pushed the bugfix/13266-definition-of-last-class-on-menu branch from 9351152 to c5d30e2 Compare January 23, 2018 12:54

arnoudhgz removed the Progress: needs update label Feb 2, 2018

orlangur added the Progress: needs update label Feb 3, 2018

orlangur requested changes Feb 12, 2018

View reviewed changes

arnoudhgz force-pushed the bugfix/13266-definition-of-last-class-on-menu branch from c5d30e2 to eff8a50 Compare February 23, 2018 13:17

arnoudhgz closed this Jul 13, 2018

arnoudhgz deleted the bugfix/13266-definition-of-last-class-on-menu branch July 13, 2018 16:27

arnoudhgz mentioned this pull request Jul 22, 2018

Bugfix/13266 Update the processing of items for the top menu #17007

Closed

4 tasks

arnoudhgz mentioned this pull request Mar 31, 2019

Make sure 'last' class is set on top menu #22071

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bugfix/13266 Update the processing of items for the top menu #13323

Bugfix/13266 Update the processing of items for the top menu #13323

Uh oh!

arnoudhgz commented Jan 23, 2018

Uh oh!

orlangur Jan 23, 2018

Uh oh!

arnoudhgz Jan 23, 2018 •

edited

Loading

Uh oh!

orlangur Feb 12, 2018

Uh oh!

arnoudhgz commented Jan 24, 2018

Uh oh!

orlangur commented Jan 24, 2018

Uh oh!

arnoudhgz commented Feb 2, 2018 •

edited

Loading

Uh oh!

orlangur commented Feb 3, 2018

Uh oh!

arnoudhgz commented Feb 3, 2018

Uh oh!

arnoudhgz commented Feb 10, 2018

Uh oh!

orlangur Feb 12, 2018

Uh oh!

orlangur commented Feb 19, 2018

Uh oh!

arnoudhgz commented Feb 23, 2018

Uh oh!

orlangur commented Jul 13, 2018

Uh oh!

arnoudhgz commented Jul 13, 2018

Uh oh!

orlangur commented Jul 15, 2018

Uh oh!

Uh oh!

Bugfix/13266 Update the processing of items for the top menu #13323

Bugfix/13266 Update the processing of items for the top menu #13323

Uh oh!

Conversation

arnoudhgz commented Jan 23, 2018

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

orlangur Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

arnoudhgz Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlangur Feb 12, 2018

Choose a reason for hiding this comment

Uh oh!

arnoudhgz commented Jan 24, 2018

Uh oh!

orlangur commented Jan 24, 2018

Uh oh!

arnoudhgz commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Feb 3, 2018

Uh oh!

arnoudhgz commented Feb 3, 2018

Uh oh!

arnoudhgz commented Feb 10, 2018

Uh oh!

orlangur Feb 12, 2018

Choose a reason for hiding this comment

Uh oh!

orlangur commented Feb 19, 2018

Uh oh!

arnoudhgz commented Feb 23, 2018

Uh oh!

orlangur commented Jul 13, 2018

Uh oh!

arnoudhgz commented Jul 13, 2018

Uh oh!

orlangur commented Jul 15, 2018

Uh oh!

Uh oh!

arnoudhgz Jan 23, 2018 •

edited

Loading

arnoudhgz commented Feb 2, 2018 •

edited

Loading