Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Automatically QueryString Params to PagerSlim #11726

Merged
merged 11 commits into from
May 24, 2022
Merged

Add Automatically QueryString Params to PagerSlim #11726

merged 11 commits into from
May 24, 2022

Conversation

Skrypt
Copy link
Contributor

@Skrypt Skrypt commented May 18, 2022

Fixes #10384

orchard.mp4

@Skrypt Skrypt requested a review from sebastienros May 18, 2022 18:12
Fix issue with first page and last page on PagerSlim
@Skrypt
Copy link
Contributor Author

Skrypt commented May 20, 2022

The more I read code in that Class the more I find optimizations to do.

@Skrypt
Copy link
Contributor Author

Skrypt commented May 20, 2022

While working on this I found out that we use also datetimes values on after and before params which makes this a little more complicated to fix. We should probably drop the datetimes unless there was a reason behind it. Also, even if we don't know the amount of total records that we have we could easily use a pageNum param instead of after and/or before. Knowing that a pagesize == Z having a PageNum implies that a page is After X and Before Y. Only the Pager needs to receive theses after and before values to evaluate a PageNum.

@jtkech
Copy link
Member

jtkech commented May 21, 2022

Don't remember all details but these after/before are integers, unique per item and representing their ordering, if a list part is orderable items have configurable Order values, otherwise we use the createdUtc ticks. In our project we have a content type having a numeric field (so editable per item) that we use to order items by using this field to set the pager after/before.

That said we also had to use the full Pager in the front end, in that case there is no after/before, only had to pass the page number, but also a TotalItemsCount property.

Now that you already did a number of changes, maybe worth to get rid of the Linq query in GetRouteData(), this for perf of pages using Pager(s).

@Skrypt
Copy link
Contributor Author

Skrypt commented May 21, 2022

I'm just trying to find a way to use consistent params because here a page can have 2 valid URI. For example.
Let's say I have a page size == 2.

?after=2 and ?before=5 will get you to the same page. This cause issues with SEO.

Another example :

?before=3 OR ?&after=0 OR /

These are all pointing to the first page which normally shouldn't require a param to be used. That would be more appropriate to not use a param for SEO.

So, if we are using a timestamp we can't always just display one param. I would have used only "after" in that case since we don't need the "before" param if we are always passing int in those params.

So, I think we will need to pass a canonical URL in the header meta of the page to make sure SEO is consistent.

Though, if we could have consistent URI in the pagers that would also fix the issue. This is why I'm looking if this can be fixed too beforehand.

We could parse timestamps and int and use a different logic with them too. This is where I'm at right now.

@jtkech
Copy link
Member

jtkech commented May 21, 2022

Yes, that's why with the 1st page of the full pager we don't add ?pagenum=1 for SEO.

Yes, with the full pager we do paging, the query skip n * size and then take a page size of items, it also needs the total count.

With the pagerSlim we have a kind of chain with afters/befores to go forward/backward, maybe to not have to do a additional query to get the total count of items.

I will think about this SEO problem in that case.

@Skrypt
Copy link
Contributor Author

Skrypt commented May 21, 2022

And the issue with timestamps is that we cannot evaluate if we are on the first page or not with them.

Here a good video to watch : https://www.youtube.com/watch?v=DIKH-q-gJNU

If I remember correctly they say we should always at least use an Id of some sort. Here using timestamps may not be the best thing. Need to understand why we used them, if they are necessary or if we can use an alternative method to reach the same goal.

@Skrypt
Copy link
Contributor Author

Skrypt commented May 24, 2022

I'm going to merge this PR and open another one for the SEO issue since it fixes the issue with the QueryString params missing.

@Skrypt Skrypt merged commit 40d3f65 into main May 24, 2022
@Skrypt Skrypt deleted the skrypt/#10384 branch May 24, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking older or newer posts button not show list of blog post but show all categories instead
3 participants