Initial KnpMenuBundle integration #99
Conversation
| $menu = $this->factory->createItem( 'root' ); | ||
| $this->addLocationsToMenu( | ||
| $menu, | ||
| $this->getSearchResults( |
There was a problem hiding this comment.
getSearchResults? well, we're searching but as this is a menu i was thinking in something like getItems instead. nothing important anyway.
|
About the whole approach, i would vote for going to a tree menú instead of two differents menús for the first and second level. To get a result like <ul>
<li>Item 1
<ul>
<li>subitem 1.1<li>
<li>subitem 1.2</li>
</ul>
</li>
<li>item 2</li>
</ul>More semantic imho and will let the user more possibilites about playing will rollovers and things like that. Thoughs on this? |
Bonus trackAs said in 1 on 1 with @bdunogier and surely in a future step, i think the selection of this bundle for building menus could benefit the new backend interface. The idea could be have a visual meu builder. Yeah, you may say this is a wordpress or drupal thing, but they're good things imho. From that builder a user could chosee nodes from the trees, adding urls to custom modules/controler or even set external links. This way, devs and editor shouldn't be worried about adding new folders as root child and developments could avoid adding those "show in menu" fields... Just an idea, but i think knpmenu bundle really give us a more solid base for this than legacy code :) ping @dpobel @bdunogier |
| <li id="nav-location-{{ location.id }}"><a href="{{ path( location ) }}">{{ ez_content_name( location.contentInfo ) }}</a></li> | ||
| {% endfor %} | ||
| </ul> | ||
| {{ knp_menu_render(menu, {'depth': 1, 'firstClass': 'firstli', 'lastClass': 'lastli', 'branchClass': 'nav'}) }} |
There was a problem hiding this comment.
i think it needs to be "branch_class" instead of branchClass. in fact that's what we got for the submenu
|
I agree with @crevillo on the fact that it is better as a tree. It is, from the end user standpoint a hierarchy. |
|
yep. it was a suggestion. just an idea. still we need more work in the KnpMenuBundle integration and in the admin interface before working on that i think. |
|
agreed :-) unsure if this should be in the pure admin part or in the editorial part I must say. I lean for more the Admin role. |
|
I'd lean towards admin as well. |
|
Well, yes, it'd maybe be better as tree. On the other hand, we need to design it first, then :-) The one that is implemented by this PR is the |
|
Yep, no problem on my side. What you mean for "design" though? Can't we Just show the tree as the Two levels are shown now? |
|
Anyway, this is a valid starting point for the Knpmenu integration imho, so i think it could be merged. The tree thing can be added after. Doing it here would probably add too much noise. |
| - export TRAVIS_BUILD_DIR="$HOME/build/ezpublish-community" | ||
| - cd "$HOME/build" | ||
| - git clone --depth 1 https://github.com/ezsystems/ezpublish-community.git | ||
| - sh $PULL_REQUEST_TRAVIS_BUILD_DIR/bin/travis/clone_ezpublish-community.sh $PR_BRANCH |
There was a problem hiding this comment.
What if we just have the following here:
# Change branch name ("master") or repository if you need something else to test your PR:
- git clone --depth 1 --single-branch --branch master https://github.com/ezsystems/ezpublish-community.git
- cd ezpublish-community
# If you just need to pull in a different version of one of the dependecies
#- composer require --no-update ezsystems/ezpublish-kernel:dev-my_branch_name
There was a problem hiding this comment.
Sounds good, I'll integrate that. Should go with a documentation page.
|
+1 with simplifications to the travis / composer changes. |
| before_install: | ||
| - export OLD_TRAVIS_BUILD_DIR="$TRAVIS_BUILD_DIR" | ||
| - echo $TRAVIS_BUILD_DIR | ||
| - export PR_TRAVIS_BUILD_DIR=$TRAVIS_BUILD_DIR |
There was a problem hiding this comment.
whats wrong with OLD_TRAVIS_BUILD_DIR? this is executed on branches as well, not just PR's ;)
There was a problem hiding this comment.
What was wrong was "old". It's the evil brother of "bak" and "manager". I'll change it to BRANCH_BUILD_DIR then :-)
|
Rebased and cleaned up, should be ready for merge. Are you fine with the .travis.yml changes, @andrerom ? |
| before_script: | ||
| - ./bin/.travis/prepare_ezpublish.sh | ||
| - rm -fR vendor/ezsystems/demobundle/EzSystems/DemoBundle | ||
| - mv "$OLD_TRAVIS_BUILD_DIR" vendor/ezsystems/demobundle/EzSystems | ||
|
|
There was a problem hiding this comment.
This new line looks strange on a list of before_script commands, same above. or?
There was a problem hiding this comment.
Gotta say I didn't wanna touch those too much...
There was a problem hiding this comment.
The fact is though that this needs to go after ./bin/.travis/prepare_ezpublish.sh, since it is what runs composer install.
Maybe this line, and therefore the rm & mv one as well, could go to before_install, but I didn't try yet.
Visible locations below the root one with a content class listed in legacy's menu.ini[MenuContentSettings.TopIdentifierList] are returned.
The ezpublish-community branch can now easily be changed in the git clone command line.
Initial KnpMenuBundle integration
Integrates a totally non-configurable two level navigation menu, similar to that of ezdemo, to the DemoBundle. Clearly needs to be rewritten, but it has the basics.
TODO
.travis.yml