Merge pull request #1637 from hydephp/simplify-the-navigation-items-c…

…lass [2.x] Simplify the navigation items class
hydephp · Mar 24, 2024 · 27fae01 · 27fae01
2 parents 3eca812 + 3e4901b
commit 27fae01
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 98 deletions.
diff --git a/packages/framework/resources/views/components/docs/sidebar-item.blade.php b/packages/framework/resources/views/components/docs/sidebar-item.blade.php
@@ -1,10 +1,11 @@
 @props(['grouped' => false])
+@php /** @var \Hyde\Framework\Features\Navigation\NavigationItem $item */ @endphp
 <li @class(['sidebar-item -ml-4 pl-4', $grouped
         ? 'active -ml-8 pl-8 bg-black/5 dark:bg-black/10'
         : 'active bg-black/5 dark:bg-black/10' => $item->isActive()
     ]) role="listitem">
     @if($item->isActive())
-        <a href="{{ $item->getRoute() }}" aria-current="true" @class([$grouped
+        <a href="{{ $item->getLink() }}" aria-current="true" @class([$grouped
             ? '-ml-8 pl-4 py-1 px-2 block text-indigo-600 dark:text-indigo-400 dark:font-medium border-l-[0.325rem] border-indigo-500 transition-colors duration-300 ease-in-out hover:bg-black/10'
             : '-ml-4 p-2 block hover:bg-black/5 dark:hover:bg-black/10 text-indigo-600 dark:text-indigo-400 dark:font-medium border-l-[0.325rem] border-indigo-500 transition-colors duration-300 ease-in-out'
         ])>
@@ -16,7 +17,7 @@
             {!! $page->getTableOfContents() !!}
         @endif
     @else
-        <a href="{{ $item->getRoute() }}" @class([$grouped
+        <a href="{{ $item->getLink() }}" @class([$grouped
             ? '-ml-8 pl-4 py-1 px-2 block border-l-[0.325rem] border-transparent transition-colors duration-300 ease-in-out hover:bg-black/10'
             : 'block -ml-4 p-2 border-l-[0.325rem] border-transparent hover:bg-black/5 dark:hover:bg-black/10'
         ])>

diff --git a/packages/framework/src/Framework/Features/Navigation/NavigationDestination.php b/packages/framework/src/Framework/Features/Navigation/NavigationDestination.php
diff --git a/packages/framework/src/Framework/Features/Navigation/NavigationGroup.php b/packages/framework/src/Framework/Features/Navigation/NavigationGroup.php
@@ -78,7 +78,7 @@ protected function addItem(NavigationItem $item): void
     protected function containsOnlyDocumentationPages(): bool
     {
         return count($this->getItems()) && collect($this->getItems())->every(function (NavigationItem $child): bool {
-            return ($child->getRoute() !== null) && $child->getRoute()->getPage() instanceof DocumentationPage;
+            return $child->getPage() instanceof DocumentationPage;
         });
     }
 }
diff --git a/packages/framework/src/Framework/Features/Navigation/NavigationItem.php b/packages/framework/src/Framework/Features/Navigation/NavigationItem.php
@@ -4,6 +4,7 @@
 
 namespace Hyde\Framework\Features\Navigation;
 
+use Hyde\Pages\Concerns\HydePage;
 use Hyde\Foundation\Facades\Routes;
 use Hyde\Hyde;
 use Hyde\Support\Models\Route;
@@ -22,7 +23,7 @@
  */
 class NavigationItem implements NavigationElement, Stringable
 {
-    protected NavigationDestination $destination;
+    protected string|Route $destination;
     protected string $label;
     protected int $priority;
 
@@ -39,14 +40,12 @@ class NavigationItem implements NavigationElement, Stringable
      */
     public function __construct(Route|string $destination, string $label, int $priority = NavigationMenu::DEFAULT, ?string $group = null)
     {
-        $this->destination = new NavigationDestination($destination);
+        $this->destination = $destination;
 
         $this->label = $label;
         $this->priority = $priority;
 
-        if ($group !== null) {
-            $this->group = static::normalizeGroupKey($group);
-        }
+        $this->group = static::normalizeGroupKey($group);
     }
 
     /**
@@ -57,50 +56,35 @@ public function __construct(Route|string $destination, string $label, int $prior
      * @param  string|null  $label  Leave blank to use the label of the route's corresponding page, if there is one tied to the route.
      * @param  string|null  $group  Leave blank to use the group of the route's corresponding page, if there is one tied to the route.
      */
-    public static function create(Route|string $destination, ?string $label = null, ?int $priority = null, ?string $group = null): self
+    public static function create(Route|string $destination, ?string $label = null, ?int $priority = null, ?string $group = null): static
     {
         if (is_string($destination) && Routes::has($destination)) {
             $destination = Routes::get($destination);
         }
 
         if ($destination instanceof Route) {
-            return new self(
-                $destination,
-                $label ?? $destination->getPage()->navigationMenuLabel(),
-                $priority ?? $destination->getPage()->navigationMenuPriority(),
-                $group ?? $destination->getPage()->navigationMenuGroup(),
-            );
+            $label ??= $destination->getPage()->navigationMenuLabel();
+            $priority ??= $destination->getPage()->navigationMenuPriority();
+            $group ??= $destination->getPage()->navigationMenuGroup();
         }
 
-        return new self($destination, $label ?? '', $priority ?? NavigationMenu::DEFAULT, $group);
+        return new static($destination, $label ?? '', $priority ?? NavigationMenu::DEFAULT, $group);
     }
 
     /**
      * Resolve a link to the navigation item.
      */
     public function __toString(): string
     {
-        return $this->getUrl();
-    }
-
-    /**
-     * Get the destination route of the navigation item. For dropdowns, this will return null.
-     *
-     * @deprecated To simplify the class, we may remove this as we probably don't need it.
-     */
-    public function getRoute(): ?Route
-    {
-        return $this->destination->getRoute();
+        return $this->getLink();
     }
 
     /**
      * Resolve the destination link of the navigation item.
-     *
-     * @deprecated May be renamed to getLink() in the future to better match its usage, and to match the Route class.
      */
-    public function getUrl(): string
+    public function getLink(): string
     {
-        return $this->destination->getLink();
+        return (string) $this->destination;
     }
 
     /**
@@ -132,12 +116,20 @@ public function getGroupKey(): ?string
         return $this->group;
     }
 
+    /**
+     * If the navigation item is a link to a routed page, get the corresponding page instance.
+     */
+    public function getPage(): ?HydePage
+    {
+        return $this->destination instanceof Route ? $this->destination->getPage() : null;
+    }
+
     /**
      * Check if the NavigationItem instance is the current page being rendered.
      */
     public function isActive(): bool
     {
-        return Hyde::currentRoute()->getLink() === $this->getUrl();
+        return Hyde::currentRoute()->getLink() === $this->getLink();
     }
 
     /** @return ($group is null ? null : string) */

diff --git a/packages/framework/tests/Unit/NavigationItemTest.php b/packages/framework/tests/Unit/NavigationItemTest.php
@@ -41,10 +41,10 @@ protected function setUp(): void
 
     public function testConstruct()
     {
-        $route = new Route(new MarkdownPage());
-        $item = new NavigationItem($route, 'Test', 500);
-
-        $this->assertSame($route, $item->getRoute());
+        $this->assertInstanceOf(NavigationItem::class, new NavigationItem('foo', 'Test'));
+        $this->assertInstanceOf(NavigationItem::class, new NavigationItem(new Route(new MarkdownPage()), 'Test'));
+        $this->assertInstanceOf(NavigationItem::class, new NavigationItem(new Route(new MarkdownPage()), 'Test', 500));
+        $this->assertInstanceOf(NavigationItem::class, new NavigationItem(new Route(new MarkdownPage()), 'Test', 500, 'foo'));
     }
 
     public function testIsInstanceOfNavigationElement()
@@ -55,42 +55,50 @@ public function testIsInstanceOfNavigationElement()
     public function testPassingRouteInstanceToConstructorUsesRouteInstance()
     {
         $route = new Route(new MarkdownPage());
-        $this->assertSame($route, (new NavigationItem($route, 'Home'))->getRoute());
+        $this->assertEquals($route, (new NavigationItem($route, 'Home'))->getPage()->getRoute());
     }
 
-    public function testPassingRouteKeyToConstructorUsesRouteInstance()
+    public function testPassingRouteKeyToConstructorUsesDestinationAsLink()
     {
-        $route = Routes::get('index');
-
-        $this->assertSame($route, (new NavigationItem('index', 'Home'))->getRoute());
+        $item = new NavigationItem('index', 'Home');
+        $this->assertNull($item->getPage());
+        $this->assertSame('index', $item->getLink());
     }
 
     public function testPassingUrlToConstructorSetsRouteToNull()
     {
         $item = new NavigationItem('https://example.com', 'Home');
-        $this->assertNull($item->getRoute());
-        $this->assertSame('https://example.com', $item->getUrl());
+        $this->assertNull($item->getPage());
+        $this->assertSame('https://example.com', $item->getLink());
     }
 
     public function testPassingUnknownRouteKeyToConstructorSetsRouteToNull()
     {
         $item = new NavigationItem('foo', 'Home');
-        $this->assertNull($item->getRoute());
-        $this->assertSame('foo', $item->getUrl());
+        $this->assertNull($item->getPage());
+        $this->assertSame('foo', $item->getLink());
     }
 
-    public function testGetDestination()
+    public function testCanGetPage()
+    {
+        $page = new MarkdownPage();
+        $item = new NavigationItem(new Route($page), 'Test', 500);
+
+        $this->assertSame($page, $item->getPage());
+    }
+
+    public function testGetPageRoute()
     {
         $route = new Route(new InMemoryPage('foo'));
         $NavigationItem = new NavigationItem($route, 'Page', 500);
 
-        $this->assertSame($route, $NavigationItem->getRoute());
+        $this->assertEquals($route, $NavigationItem->getPage()->getRoute());
     }
 
     public function testGetLink()
     {
         $NavigationItem = new NavigationItem(new Route(new InMemoryPage('foo')), 'Page', 500);
-        $this->assertSame('foo.html', $NavigationItem->getUrl());
+        $this->assertSame('foo.html', $NavigationItem->getLink());
     }
 
     public function testGetLabel()
@@ -113,10 +121,12 @@ public function testGetGroup()
 
     public function testFromRoute()
     {
-        $route = new Route(new MarkdownPage());
+        $page = new MarkdownPage();
+        $route = new Route($page);
         $item = NavigationItem::create($route);
 
-        $this->assertSame($route, $item->getRoute());
+        $this->assertSame($page, $item->getPage());
+        $this->assertEquals($route, $item->getPage()->getRoute());
     }
 
     public function testToString()
@@ -130,8 +140,8 @@ public function testCreateWithLink()
     {
         $item = NavigationItem::create('foo', 'bar');
 
-        $this->assertNull($item->getRoute());
-        $this->assertSame('foo', $item->getUrl());
+        $this->assertNull($item->getPage());
+        $this->assertSame('foo', $item->getLink());
         $this->assertSame('bar', $item->getLabel());
         $this->assertSame(500, $item->getPriority());
     }
@@ -146,19 +156,38 @@ public function testCreate()
         $route = Routes::get('404');
         $item = NavigationItem::create($route, 'foo');
 
-        $this->assertSame($route, $item->getRoute());
+        $this->assertSame($route->getPage(), $item->getPage());
         $this->assertSame('foo', $item->getLabel());
         $this->assertSame(999, $item->getPriority());
+        $this->assertEquals($route, $item->getPage()->getRoute());
+        $this->assertSame('404.html', $item->getLink());
+        $this->assertSame('404.html', (string) $item);
+        $this->assertNull($item->getGroupKey());
     }
 
     public function testForIndexRoute()
     {
         $route = Routes::get('index');
-        $item = NavigationItem::create($route, 'foo');
+        $item = NavigationItem::create($route);
 
-        $this->assertSame($route, $item->getRoute());
-        $this->assertSame('foo', $item->getLabel());
+        $this->assertSame($route->getPage(), $item->getPage());
+        $this->assertSame('Home', $item->getLabel());
         $this->assertSame(0, $item->getPriority());
+        $this->assertEquals($route, $item->getPage()->getRoute());
+        $this->assertSame('index.html', $item->getLink());
+        $this->assertSame('index.html', (string) $item);
+        $this->assertNull($item->getGroupKey());
+    }
+
+    public function testCreateWithLabels()
+    {
+        $route = Routes::get('404');
+        $item = NavigationItem::create($route, 'foo');
+        $this->assertSame('foo', $item->getLabel());
+
+        $route = Routes::get('index');
+        $item = NavigationItem::create($route);
+        $this->assertSame('Home', $item->getLabel());
     }
 
     public function testCreateWithRouteKey()
@@ -171,14 +200,22 @@ public function testCreateWithRouteKey()
 
     public function testCreateWithMissingRouteKey()
     {
-        $this->assertNull(NavigationItem::create('foo', 'foo')->getRoute());
+        $this->assertNull(NavigationItem::create('foo', 'foo')->getPage());
     }
 
     public function testCreateWithCustomPriority()
     {
         $this->assertSame(100, NavigationItem::create(Routes::get('index'), 'foo', 100)->getPriority());
     }
 
+    public function testPassingRouteKeyToStaticConstructorUsesRouteInstance()
+    {
+        $route = Routes::get('index');
+        $item = NavigationItem::create('index', 'Home');
+        $this->assertNotNull($item->getPage());
+        $this->assertSame($route->getPage(), $item->getPage());
+    }
+
     public function testRouteBasedNavigationItemDestinationsAreResolvedRelatively()
     {
         Render::swap(Mockery::mock(RenderData::class, [