Add External link in admin navigation #6328
Conversation
|
That's the php part for the moment. I've implemented an NavigationItemInterface to handle types in the registry and adminController. Feel free to comment and propose an other approach. |
|
Moving on the front part. |
alexander-schranz
changed the title
There was a problem hiding this comment.
@sebheitzmann Thanks for working on this, I did a quick review and we maybe need to discuss the Interface, but at all it looks good to that you could start with the frontend part.
| public function hasLink(); | ||
| public function getView(); |
There was a problem hiding this comment.
@sulu/core-team how we define a interface here. In the views we only have in the interface which really relevant for all types and use options array to provide type specific options. I'm thinking about here that the normal NavigationItem has only a setView and getView and will write this into an options array like we do it for views e.g.:
public function setView(string $view): void
{
return $this->options['view'] = $view;
}and the ExternalNavigationItem then with:
public function setUrl(string $url): void
{
return $this->options['url'] = $url;
}And so the NavigationItemInterface instead of having getView it will have a getOptions and getType method.
There was a problem hiding this comment.
I have done this because they are used in the Registry. But maybe it should be avoided
There was a problem hiding this comment.
i am not sure if i would separate internal and external navigation items into separate classes. they share a lot of things (eg. label, icon, position), right?
maybe it would be better to add a type on the NavigationItem class? we could add a ViewNavigationItemBuilder and a ExternalLinkNavigationItemBuilder to create and validate different types of navigation items. this would be consistent to the existing View concept.
There was a problem hiding this comment.
Sounds good for me to have the same concept as for Views here. We would need a NavigationBuilderFactory then like the ViewBuilderFactory.
There was a problem hiding this comment.
For backward compatibility the NavigationItemCollection would need to be changed to accept NavigationItemBuilder and NavigationItem then.
There was a problem hiding this comment.
I'm sorry but I don't really understand what i should do with the ViewBuilderFactory. I can merge the NavigationItem and the ExternalNavigation if you want.
| @@ -76,7 +76,7 @@ private function loadNavigationItems(): void | |||
| } | |||
|
|
|||
| $navigationItems = \array_filter($navigationItemCollection->all(), function($navigationItem) { | |||
| return $navigationItem->getChildren() || $navigationItem->getView(); | |||
| return $navigationItem->getChildren() || $navigationItem->hasLink(); | |||
There was a problem hiding this comment.
Interesting didn't know we filter this out here this way. Need to check if the whole array_filter maybe could be removed so we don't need to have something like hasLink here.
There was a problem hiding this comment.
Don't really understand why we sould filter the items. Is it possible to have items without view ? If they are hidden i think it's useless. I will correct this.
There was a problem hiding this comment.
i think the items are filtered to filter out empty parents. for example, if a user does not have permissions for accounts and for contacts, the Contact navigation item would be empty
There was a problem hiding this comment.
ok, I will revert my change to keep the getChildren filter. But the getView is useless no ?
There was a problem hiding this comment.
No, the getView is needed if there is no children. Instead all the leaf link will be removed
There was a problem hiding this comment.
maybe a shouldShow() function on the interface ?
There was a problem hiding this comment.
If we add a special GroupNavigationItem we would not need a shouldShow instead we could check here: return !$navigationItem instanceOf GroupNavigatinItemBuilder || count(!$navigationItem->getChildren());. Because a ViewNavigationItem without view or a ExternalNavigationItem without Url would never exists and so don't need to be filtered out.
| public function copyChildless() | ||
| { | ||
| $new = $this->copyWithName(); | ||
| $new->setView($this->getView()); |
There was a problem hiding this comment.
Think this should be $new->setUrl($this->getUrl());
There was a problem hiding this comment.
I thing that an external link should not have children. So maybe this function should be removed. ( but corrected )
There was a problem hiding this comment.
Maybe we introduce a speical GroupNavigationItem because if we name the other one ViewNavigationItem it also does not make sense that it can have children.
| return $this->getUrl() !== null; | ||
| } | ||
|
|
||
| public function getView() |
There was a problem hiding this comment.
The external should know nothing about view, need to discuss the Interface with the other core team members.
There was a problem hiding this comment.
The getView is used in the NavigationRegistry ( line 108 )
// add child views
$mainView = $navigationItem->getView();
if ($mainView) {
$mainPath = $this->viewRegistry->findViewByName($mainView)->getPath();
if ('/' !== $mainPath) {
foreach ($this->viewRegistry->getViews() as $view) {
if (0 === \strpos($view->getPath(), $mainPath)) {
$navigationItem->addChildView($view->getName());
}
}
}
}Maybe this should be done in the Item and not in the registry.
There was a problem hiding this comment.
I thinks that it's for the "active" feature.
src/Sulu/Bundle/AdminBundle/Admin/Navigation/NavigationExternalItem.php
Outdated
Show resolved
Hide resolved
|
|
alexander-schranz
added
the
hacktoberfest-accepted
|
|
||
| namespace Sulu\Bundle\AdminBundle\Admin\Navigation; | ||
|
|
||
| class NavigationExternalItem implements \Iterator, NavigationItemInterface |
There was a problem hiding this comment.
This class is missing proper type information. We should use the php typehinting:
/**
* @var string
*/
protected $id;will become
protected string $id;That way we have help from php checking the types. Same with arguments and return types.
What's in this PR?
Permit the admin navitation to link to external url
Why?
Link to external tools ( google analytics, or other web companion )
Example Usage
To Do