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

Set right order of grouped menu entries #7219

Merged
merged 6 commits into from Dec 4, 2017

Conversation

Projects
None yet
4 participants
@JarJak
Contributor

JarJak commented Nov 28, 2017

Previously, all grouped menu entries were put at the bottom of menu (as "other" menu).

Now they can be mixed with non-grouped entries with proper order as defined in contenttypes.yml.

I will add tests later (or update if some of them would fail).

Discussed on Slack with @bobdenotter

@GawainLynch

Update tests please

@GawainLynch

Changed logic still not covered in tests.

private function prepareGroupedMenu(MenuEntry $contentRoot, Bag $contentType)
{
if ($contentType->get('show_in_menu') === true) {
return;

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

Dead code, you check this above.

foreach ($contentTypes as $name => $contentType) {
$this->addContentType($mainContentRoot, $name, $contentType);
foreach ($this->contentTypes as $name => $contentType) {
if ($contentType['show_in_menu'] === true) {

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

$this->contentTypes is a recursive Bag, ergo $contentType is a Bag, use the getter please.

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

Also, you're looking for the exception to the logical flow, it would be cleaner to flip the test, i.e. !== true

foreach ($this->contentTypes as $name => $contentType) {
if ($contentType['show_in_menu'] === true) {
$this->addContentType($mainContentRoot, $name, $contentType);
} else {

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

Remove the else and add a continue; above

$key = 'other';
} else {
$key = $contentType->get('show_in_menu');
}

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

$key = $contentType->get('show_in_menu') ?: 'other;

return $root;
}
private function prepareGroupedMenu(MenuEntry $contentRoot, Bag $contentType)

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

Docblock

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

This is an "add" not a "prepare", and move below the addContentType() method

foreach ($contentRoot->get('grouped')->children() as $groupName => $groupMenu) {
foreach ($groupMenu->children() as $name => $contentMenu) {
if ($contentMenu->isGroup()) {
foreach ($contentMenu->children() as $name => $contentMenu) {

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

$name and $contentMenu are already in use … this is really bad practice.

foreach ($contentMenu->children() as $name => $contentMenu) {
if ($contentTypes->getPath($name . '/singleton')) {
$this->addSingleton($contentMenu, $name);
}

This comment has been minimized.

@GawainLynch

GawainLynch Nov 29, 2017

Contributor

Nesting is too deep, restructure and simplify the logic, please.

[
MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('entry2'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

[
MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('group1')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

)->parent()
->add(MenuEntry::create('entry2'))->parent()
->add(MenuEntry::create('entry3'))->parent()
->add(MenuEntry::create('group2')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('group2entry2'))->parent()
)->parent()
->add(MenuEntry::create('entry4'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

return [
'name' => $menuEntry->getName(),
'group' => $menuEntry->isGroup(),
'children' => array_map(function($child) {

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 1 space after FUNCTION keyword; 0 found

}))
];
}
}

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 1 newline at end of file; 0 found

[
MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

,
MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

],
[
MenuEntry::create('main')
->add(MenuEntry::create('entry1')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('entry1')
->add(MenuEntry::create('singleton'))->parent()
)->parent()
->add(MenuEntry::create('entry2')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('entry2')
->add(MenuEntry::create('singleton'))->parent()
)->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

)->parent()
->add(MenuEntry::create('other')
->setGroup(true)
->add(MenuEntry::create('othergroupentry1')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('othergroupentry1')
->add(MenuEntry::create('singleton'))->parent()
)->parent()
->add(MenuEntry::create('othergroupentry2')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('entry2'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

[
MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('group1')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('group1')
->setGroup(true)
->add(MenuEntry::create('group1entry1'))->parent()
->add(MenuEntry::create('group1entry2')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('singleton'))->parent()
)->parent()
)->parent()
->add(MenuEntry::create('entry2')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('singleton'))->parent()
)->parent()
->add(MenuEntry::create('entry3'))->parent()
->add(MenuEntry::create('group2')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('group2entry2'))->parent()
)->parent()
->add(MenuEntry::create('entry4'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

,
MenuEntry::create('main')
->add(MenuEntry::create('entry1'))->parent()
->add(MenuEntry::create('group1')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

)->parent()
->add(MenuEntry::create('entry2'))->parent()
->add(MenuEntry::create('entry3'))->parent()
->add(MenuEntry::create('group2')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

->add(MenuEntry::create('group2entry2'))->parent()
)->parent()
->add(MenuEntry::create('entry4'))->parent()
->add(MenuEntry::create('other')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

return [
'name' => $menuEntry->getName(),
'group' => $menuEntry->isGroup(),
'children' => array_map(function($child) {

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 1 space after FUNCTION keyword; 0 found

}))
];
}
}

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 1 newline at end of file; 0 found

})),
];
}
}

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 1 newline at end of file; 0 found

],
[
MenuEntry::create('main')
->add(MenuEntry::create('entry1')

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 0 spaces before closing bracket; newline found

})),
];
}
}

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Expected 1 newline at end of file; 0 found

$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add(
$expectedMain)

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

  • Multi-line function call not indented correctly; expected 12 spaces but found 16
  • Closing parenthesis of a multi-line function call must be on a line by itself
$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add(
$expectedMain)
->parent())

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Closing parenthesis of a multi-line function call must be on a line by itself

$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add(
$expectedMain)

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

  • Multi-line function call not indented correctly; expected 12 spaces but found 16
  • Closing parenthesis of a multi-line function call must be on a line by itself
$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add(
$expectedMain)
->parent())

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Closing parenthesis of a multi-line function call must be on a line by itself

;
$entry = MenuEntry::create('root')->add(
MenuEntry::create('content')->add(
$entryMain)

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

  • Multi-line function call not indented correctly; expected 12 spaces but found 16
  • Closing parenthesis of a multi-line function call must be on a line by itself
$entry = MenuEntry::create('root')->add(
MenuEntry::create('content')->add(
$entryMain)
->parent())

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Closing parenthesis of a multi-line function call must be on a line by itself

$result = $ac->build(MenuEntry::create('root'));
$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add($expectedMain)->parent()

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Multi-line function call not indented correctly; expected 12 spaces but found 16

$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add($expectedMain)->parent()
)

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Multi-line function call not indented correctly; expected 8 spaces but found 12

$re = new RecentlyEdited($em, $this->createMock(ParseDown::class));
$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add($expectedMain)->parent()

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Multi-line function call not indented correctly; expected 12 spaces but found 16

$expected = MenuEntry::create('root')->add(
MenuEntry::create('content')->add($expectedMain)->parent()
)

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Multi-line function call not indented correctly; expected 8 spaces but found 12

->parent()
;
$entry = MenuEntry::create('root')->add(
MenuEntry::create('content')->add($entryMain)->parent()

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Multi-line function call not indented correctly; expected 12 spaces but found 16

;
$entry = MenuEntry::create('root')->add(
MenuEntry::create('content')->add($entryMain)->parent()
)

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Nov 29, 2017

Multi-line function call not indented correctly; expected 8 spaces but found 12

@GawainLynch GawainLynch changed the base branch from 3.4 to 3.5 Dec 4, 2017

JarJak added some commits Nov 28, 2017

@GawainLynch

Thanks mate, and extra thanks for the unit tests 👍

@GawainLynch GawainLynch merged commit 7bafdd3 into bolt:3.5 Dec 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bobdenotter

This comment has been minimized.

Member

bobdenotter commented Dec 4, 2017

Yay, teamwork! Happy we landed this improvement! 👏

@GawainLynch GawainLynch added this to the Bolt 3.5 - Feature release milestone Dec 8, 2017

@JarJak JarJak deleted the JarJak:patch-7 branch Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment