Catalog pagination doesn't preserve type and brand choice

[yigith]

Related File: https://github.com/dotnet-architecture/eShopOnWeb/blob/master/src/Web/Pages/Shared/_pagination.cshtml

The next and previous links forget about the user selections of type and brand on the next page. Do you think this is a missing feature?

Note: In my own implementation, I passed all the query string parameters (which may not be a safe solution) to the route data along with pageId.

Note 2: In order to reproduce the problem, I increased the amount of seed values.

[efleming18]

Hey @yigith - Yep, looks like a bug. I was able to reproduce it once I increased the amount of seed data as well.

If you would like to work on a fix, that would be great if not I'm happy to get to it soon!

[yigith]

I would like to work on a fix but if it is OK I would like to ask you about the right approach to fix this problem.

The following code fixes the problem by carrying all the query string data existing in the request to the 'previous' and 'next' links. In this way, pagination links will work even with other filters such as a search query string parameter if exists. But the problem with this approach is it will carry a query string which is meant to exist only at the current page. This approach has no filter or control over which query string parameters should be transferred to the next/previous page. File: _pagination.cshtml

@model PaginationInfoViewModel
@{
    var prevRouteData = Context.Request.Query.ToDictionary(x => x.Key, x => x.Value.ToString());
    if (prevRouteData.ContainsKey("pageId"))
        prevRouteData.Remove("pageId");
    prevRouteData.Add("pageId", (Model.ActualPage - 1).ToString());
    var nextRouteData = Context.Request.Query.ToDictionary(x => x.Key, x => x.Value.ToString());
    if (nextRouteData.ContainsKey("pageId"))
        nextRouteData.Remove("pageId");
    nextRouteData.Add("pageId", (Model.ActualPage + 1).ToString());
}
<div class="esh-pager">
    <div class="container-fluid">
        <article class="esh-pager-wrapper row">
            <nav>
                <div class="col-md-2 col-xs-12">
                    <a class="esh-pager-item-left esh-pager-item--navigable esh-pager-item @Model.Previous"
                       id="Previous"
                       asp-all-route-data="prevRouteData"
                       aria-label="Previous">
                        Previous
                    </a>
                </div>

                <div class="col-md-8 col-xs-12">
                    <span class="esh-pager-item">
                        Showing @Model.ItemsPerPage of @Model.TotalItems products - Page @(Model.ActualPage + 1) - @Model.TotalPages
                    </span>
                </div>

                <div class="col-md-2 col-xs-12">
                    <a class="esh-pager-item-right esh-pager-item--navigable esh-pager-item @Model.Next"
                       id="Next"
                       asp-all-route-data="nextRouteData"
                       aria-label="Next">
                        Next
                    </a>
                </div>
            </nav>
        </article>
    </div>
</div>

I believe seperating _pagination partial to the shared folder means that this should work with any items collection which provides a PaginationInfoViewModel.

Regarding this, a possible solution could be providing a route data dictionary in the PaginationViewModel which contains the query string parameters should be carried to the next page and it could be done in the method CatalogViewModelService.GetCatalogItems.

Another approach could be writing a tag helper providing attributes like asp-route-*.

Third approach is passing the required route data to _pagination partial is through a ViewData property which will be loaded on the Index.cshtml (or whereever the _pagination partial will be used)

[efleming18]

Hey @yigith - for the purposes of this application, I actually think the code snippet you provided above is the best option. I pulled it down and did some testing with it and the cases I went through, seemed to work as expected.

But the problem with this approach is it will carry a query string which is meant to exist only at the current page. This approach has no filter or control over which query string parameters should be transferred to the next/previous page.

For the Catalog pagination here, I would expect that all the query string params / filters should all be transferred to the next/previous page. If there arises another place in the app where we need a different type of pagination, I would expect to create another partial to handle that depending on if the scenario calls for only a subset of query string params/filters to be applied and controlled.

So, I think the code snippet you have above is great! If you want to open a PR for that we can get it merged in.

[efleming18]

Resolved by attached PR.