Reviewed and refactored next/previous navigation button implementation

- Updated styling to include item name.
- Extracted used text to translations.
- Updated the design to better suit the surrounding blocks.
- Removed newly added model/repo methods.
- Moved core logic out of controller and instead into a "NextPreviousContentLocator"
helper with re-uses the output from the book-tree generation.
- Also added the system to chapters.

For #2511

app/Entities/Models/Page.php

@@ -138,13 +138,4 @@ class Page extends BookChild
         $refreshed->html = (new PageContent($refreshed))->render();
         return $refreshed;
     }
-    /**
-     * Get the parent chapter ID.
-     */
-    public function getParentChapter()
-    {
-        $chapterId = $this->chapter()->visible()
-        ->get('id');
-        return $chapterId;
-    }
 }

app/Entities/Repos/PageRepo.php

@@ -468,10 +468,4 @@ class PageRepo
             ->where('page_id', '=', $page->id)
             ->orderBy('created_at', 'desc');
     }
-    /**
-     * Get page details by chapter ID.
-     */
-    public function getPageByChapterID(int $id){
-        return Page::visible()->where('chapter_id', '=', $id)->get(['id','slug']);
-    }
 }

app/Entities/Tools/NextPreviousContentLocator.php

@@ -0,0 +1,69 @@
+<?php namespace BookStack\Entities\Tools;
+
+use BookStack\Entities\Models\BookChild;
+use BookStack\Entities\Models\Entity;
+use Illuminate\Support\Collection;
+
+/**
+ * Finds the next or previous content of a book element (page or chapter).
+ */
+class NextPreviousContentLocator
+{
+    protected $relativeBookItem;
+    protected $flatTree;
+    protected $currentIndex = null;
+
+    /**
+     * NextPreviousContentLocator constructor.
+     */
+    public function __construct(BookChild $relativeBookItem, Collection $bookTree)
+    {
+        $this->relativeBookItem = $relativeBookItem;
+        $this->flatTree = $this->treeToFlatOrderedCollection($bookTree);
+        $this->currentIndex = $this->getCurrentIndex();
+    }
+
+    /**
+     * Get the next logical entity within the book hierarchy.
+     */
+    public function getNext(): ?Entity
+    {
+        return $this->flatTree->get($this->currentIndex + 1);
+    }
+
+    /**
+     * Get the next logical entity within the book hierarchy.
+     */
+    public function getPrevious(): ?Entity
+    {
+        return $this->flatTree->get($this->currentIndex - 1);
+    }
+
+    /**
+     * Get the index of the current relative item.
+     */
+    protected function getCurrentIndex(): ?int
+    {
+        $index = $this->flatTree->search(function (Entity $entity) {
+            return get_class($entity) === get_class($this->relativeBookItem)
+                && $entity->id === $this->relativeBookItem->id;
+        });
+        return $index === false ? null : $index;
+    }
+
+    /**
+     * Convert a book tree collection to a flattened version
+     * where all items follow the expected order of user flow.
+     */
+    protected function treeToFlatOrderedCollection(Collection $bookTree): Collection
+    {
+        $flatOrdered = collect();
+        /** @var Entity $item */
+        foreach ($bookTree->all() as $item) {
+            $flatOrdered->push($item);
+            $childPages = $item->visible_pages ?? [];
+            $flatOrdered = $flatOrdered->concat($childPages);
+        }
+        return $flatOrdered;
+    }
+}

app/Http/Controllers/ChapterController.php

@@ -4,6 +4,7 @@ use BookStack\Actions\View;
 use BookStack\Entities\Models\Book;
 use BookStack\Entities\Tools\BookContents;
 use BookStack\Entities\Repos\ChapterRepo;
+use BookStack\Entities\Tools\NextPreviousContentLocator;
 use BookStack\Entities\Tools\PermissionsUpdater;
 use BookStack\Exceptions\MoveOperationException;
 use BookStack\Exceptions\NotFoundException;
@@ -65,6 +66,7 @@ class ChapterController extends Controller
 
         $sidebarTree = (new BookContents($chapter->book))->getTree();
         $pages = $chapter->getVisiblePages();
+        $nextPreviousLocator = new NextPreviousContentLocator($chapter, $sidebarTree);
         View::incrementFor($chapter);
 
         $this->setPageTitle($chapter->getShortName());
@@ -73,7 +75,9 @@ class ChapterController extends Controller
             'chapter' => $chapter,
             'current' => $chapter,
             'sidebarTree' => $sidebarTree,
-            'pages' => $pages
+            'pages' => $pages,
+            'next' => $nextPreviousLocator->getNext(),
+            'previous' => $nextPreviousLocator->getPrevious(),
         ]);
     }
 

app/Http/Controllers/PageController.php

@@ -2,6 +2,7 @@
 
 use BookStack\Actions\View;
 use BookStack\Entities\Tools\BookContents;
+use BookStack\Entities\Tools\NextPreviousContentLocator;
 use BookStack\Entities\Tools\PageContent;
 use BookStack\Entities\Tools\PageEditActivity;
 use BookStack\Entities\Models\Page;
@@ -142,39 +143,8 @@ class PageController extends Controller
             $page->load(['comments.createdBy']);
         }
 
-        $chapterId = $page->getParentChapter();
-        $allPageSlugs = $this->pageRepo->getPageByChapterID($chapterId[0]->id);
-        $pos = 0;
-        foreach ($allPageSlugs as $slug){
-            if($pageSlug === $slug->slug){
-                $currPagePos = $pos;
-            }
-            $pos++;
-            $pageUrl = $this->pageRepo->getBySlug($bookSlug, $slug->slug);
-            $urlLink[] = $pageUrl->getUrl();
-        }
-        for($i=0; $i <= $currPagePos; $i++){
-            $nextCount = $i+1;
-            $prevCount = $i-1;
-            $prevPage = '#';
-            $nextPage = '#';
-            if($nextCount < count($urlLink)){
-                $nextPage = $urlLink[$nextCount];
-            }
-            if($currPagePos == $i && $currPagePos != 0){
-                $prevPage = $urlLink[$prevCount];    
-            }
-        }
+        $nextPreviousLocator = new NextPreviousContentLocator($page, $sidebarTree);
 
-        $disablePrev = "";
-        $disableNxt = "";
-        if($prevPage == "#"){
-            $disablePrev = "disabled";
-        }
-        if($nextPage == "#"){
-            $disableNxt = "disabled";
-        }
-        
         View::incrementFor($page);
         $this->setPageTitle($page->getShortName());
         return view('pages.show', [
@@ -184,10 +154,8 @@ class PageController extends Controller
             'sidebarTree' => $sidebarTree,
             'commentsEnabled' => $commentsEnabled,
             'pageNav' => $pageNav,
-            'prevPage' => $prevPage,
-            'nextPage' => $nextPage,
-            'disablePrev' => $disablePrev,
-            'disableNxt' => $disableNxt
+            'next' => $nextPreviousLocator->getNext(),
+            'previous' => $nextPreviousLocator->getPrevious(),
         ]);
     }
 
@@ -280,8 +248,8 @@ class PageController extends Controller
 
         $updateTime = $draft->updated_at->timestamp;
         return response()->json([
-            'status'    => 'success',
-            'message'   => trans('entities.pages_edit_draft_save_at'),
+            'status' => 'success',
+            'message' => trans('entities.pages_edit_draft_save_at'),
             'timestamp' => $updateTime
         ]);
     }
@@ -304,7 +272,7 @@ class PageController extends Controller
     {
         $page = $this->pageRepo->getBySlug($bookSlug, $pageSlug);
         $this->checkOwnablePermission('page-delete', $page);
-        $this->setPageTitle(trans('entities.pages_delete_named', ['pageName'=>$page->getShortName()]));
+        $this->setPageTitle(trans('entities.pages_delete_named', ['pageName' => $page->getShortName()]));
         return view('pages.delete', [
             'book' => $page->book,
             'page' => $page,
@@ -320,7 +288,7 @@ class PageController extends Controller
     {
         $page = $this->pageRepo->getById($pageId);
         $this->checkOwnablePermission('page-update', $page);
-        $this->setPageTitle(trans('entities.pages_delete_draft_named', ['pageName'=>$page->getShortName()]));
+        $this->setPageTitle(trans('entities.pages_delete_draft_named', ['pageName' => $page->getShortName()]));
         return view('pages.delete', [
             'book' => $page->book,
             'page' => $page,
@@ -415,7 +383,7 @@ class PageController extends Controller
         try {
             $parent = $this->pageRepo->move($page, $entitySelection);
         } catch (Exception $exception) {
-            if ($exception instanceof  PermissionsException) {
+            if ($exception instanceof PermissionsException) {
                 $this->showPermissionError();
             }
 
@@ -459,7 +427,7 @@ class PageController extends Controller
         try {
             $pageCopy = $this->pageRepo->copy($page, $entitySelection, $newName);
         } catch (Exception $exception) {
-            if ($exception instanceof  PermissionsException) {
+            if ($exception instanceof PermissionsException) {
                 $this->showPermissionError();
             }
 
@@ -480,7 +448,7 @@ class PageController extends Controller
         $page = $this->pageRepo->getBySlug($bookSlug, $pageSlug);
         $this->checkOwnablePermission('restrictions-manage', $page);
         return view('pages.permissions', [
-            'page'  => $page,
+            'page' => $page,
         ]);
     }
 

resources/lang/en/common.php

@@ -42,6 +42,8 @@ return [
     'fullscreen' => 'Fullscreen',
     'favourite' => 'Favourite',
     'unfavourite' => 'Unfavourite',
+    'next' => 'Next',
+    'previous' => 'Previous',
 
     // Sort Options
     'sort_options' => 'Sort Options',

resources/sass/_blocks.scss

@@ -190,7 +190,7 @@
   padding: $-m $-xxl;
   margin-inline-start: auto;
   margin-inline-end: auto;
-  margin-bottom: $-xl;
+  margin-bottom: $-l;
   overflow: initial;
   min-height: 60vh;
   &.auto-height {
@@ -216,6 +216,21 @@
   }
 }
 
+.outline-hover {
+  border: 1px solid transparent !important;
+  &:hover {
+    border: 1px solid rgba(0, 0, 0, 0.1) !important;
+  }
+}
+
+.fade-in-when-active {
+  opacity: 0.6;
+  transition: opacity ease-in-out 120ms;
+  &:hover, &:focus-within {
+    opacity: 1;
+  }
+}
+
 /**
  * Tags
  */

resources/sass/_layout.scss

@@ -62,9 +62,6 @@
 }
 
 @include smaller-than($m) {
-  .grid.third.prev-next:not(.no-break) {
-    grid-template-columns: 1fr 1fr 1fr;
-  }
   .grid.third:not(.no-break) {
     grid-template-columns: 1fr 1fr;
   }
@@ -84,24 +81,12 @@
   .grid.right-focus.reverse-collapse > *:nth-child(1) {
     order: 1;
   }
-  .grid.third:not(.no-break) .text-m-left {
-    margin-left: 20%;
-  }
-  .grid.third:not(.no-break) .text-m-right {
-    margin-left: 45%;
-  }
 }
 
 @include smaller-than($s) {
   .grid.third:not(.no-break) {
     grid-template-columns: 1fr;
   }
-  .grid.third:not(.no-break) .text-m-left {
-    margin-left: 18%;
-  }
-  .grid.third:not(.no-break) .text-m-right {
-    margin-left: 20%;
-  }
 }
 
 @include smaller-than($xs) {
@@ -227,6 +212,13 @@ body.flexbox {
   }
 }
 
+/**
+ * Border radiuses
+ */
+.rounded {
+  border-radius: 4px;
+}
+
 /**
  * Inline content columns
  */
@@ -381,8 +373,4 @@ body.flexbox {
     margin-inline-start: 0;
     margin-inline-end: 0;
   }
-}
-
-.prev-next-btn {
-  height: 50px;
-}
+}

resources/sass/_lists.scss

@@ -441,12 +441,8 @@ ul.pagination {
     background-color: rgba(0, 0, 0, 0.1);
     border-radius: 4px;
   }
-  &.outline-hover {
-    border: 1px solid transparent;
-  }
   &.outline-hover:hover {
     background-color: transparent;
-    border-color: rgba(0, 0, 0, 0.1);
   }
   &:focus {
     @include lightDark(background-color, #eee, #222);

resources/sass/_text.scss

@@ -112,10 +112,11 @@ a {
   }
 }
 
-a.disabled {
-  pointer-events: none;
-  cursor: default;
-  opacity: 0.6;
+a.no-link-style {
+  color: inherit;
+  &:hover {
+    text-decoration: none;
+  }
 }
 
 .blended-links a {
@@ -141,6 +142,9 @@ hr {
   &.faded {
     background-image: linear-gradient(to right, #FFF, #e3e0e0 20%, #e3e0e0 80%, #FFF);
   }
+  &.darker {
+    @include lightDark(background, #DDD, #666);
+  }
   &.margin-top, &.even {
     margin-top: $-l;
   }

resources/views/chapters/show.blade.php

@@ -52,6 +52,8 @@
         @include('partials.entity-search-results')
     </main>
 
+    @include('partials.entity-sibling-navigation', ['next' => $next, 'previous' => $previous])
+
 @stop
 
 @section('right')

resources/views/pages/show.blade.php

@@ -16,21 +16,17 @@
             @include('pages.page-display')
         </div>
     </main>
-       
-    <div class="prev-next-btn">
-        <div class="grid third no-row-gap prev-next">
-            <div class="text-m-left">
-                <a class="{{ $disablePrev }}" href="{{ $prevPage }}">Previous Page</a>
-            </div>
-            <div></div>
-            <div class="text-m-right">
-                <a class="{{ $disableNxt }}" href="{{ $nextPage }}">Next Page</a>
-            </div>
-        </div>
-    </div>
+
+    @include('partials.entity-sibling-navigation', ['next' => $next, 'previous' => $previous])
 
     @if ($commentsEnabled)
-        <div class="container small p-none comments-container mb-l print-hidden">
+        @if(($previous || $next))
+            <div class="px-xl">
+                <hr class="darker">
+            </div>
+        @endif
+
+        <div class="px-xl comments-container mb-l print-hidden">
             @include('comments.comments', ['page' => $page])
             <div class="clearfix"></div>
         </div>

resources/views/partials/entity-sibling-navigation.blade.php

@@ -0,0 +1,28 @@
+<div class="grid half collapse-xs items-center mb-m px-m no-row-gap fade-in-when-active print-hidden">
+    <div>
+        @if($previous)
+            <a href="{{  $previous->getUrl()  }}" class="outline-hover no-link-style block rounded">
+                <div class="px-m pt-xs text-muted">{{ trans('common.previous') }}</div>
+                <div class="inline-block">
+                    <div class="icon-list-item no-hover">
+                        <span class="text-{{ $previous->getType() }} ">@icon($previous->getType())</span>
+                        <span>{{ $previous->getShortName(48) }}</span>
+                    </div>
+                </div>
+            </a>
+        @endif
+    </div>
+    <div>
+        @if($next)
+            <a href="{{  $next->getUrl()  }}" class="outline-hover no-link-style block rounded text-xs-right">
+                <div class="px-m pt-xs text-muted text-xs-right">{{ trans('common.next') }}</div>
+                <div class="inline block">
+                    <div class="icon-list-item no-hover">
+                        <span class="text-{{ $next->getType() }} ">@icon($next->getType())</span>
+                        <span>{{ $next->getShortName(48) }}</span>
+                    </div>
+                </div>
+            </a>
+        @endif
+    </div>
+</div>