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

Explicit paging #216

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Explicit paging #216

wants to merge 5 commits into from

Conversation

azimonti
Copy link

@azimonti azimonti commented Dec 13, 2024

check list

  • Add test cases for the changes.
  • Passed the CI test.

Description

Adding a flag for explicitPaging introduced in the version 4.0.0 of hexo-pagination. hexojs/hexo-pagination#114
Adding the option to have the last page to be latest which is convenient in case of reverse pagination.

Additional information

@azimonti azimonti marked this pull request as draft December 13, 2024 10:54
@azimonti azimonti marked this pull request as ready for review December 13, 2024 10:58
@coveralls
Copy link

Coverage Status

coverage: 94.656% (-5.3%) from 100.0%
when pulling 59ebcaa on azimonti:explicit-paging
into 87a4772 on hexojs:master.

Copy link
Member

@uiolee uiolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing unit tests. If convenient, please add some test cases

package.json Outdated Show resolved Hide resolved
lib/generator.js Outdated
perPage,
layout: ['archive', 'index'],
format: paginationDir + '/%d/',
explicitPaging: (config.archive_generator.explicit_paging || config.archive_generator.overwrite_latest || false),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think overwrite_latest should not be added here, as this may confuse users. It makes it ambiguous whether to enable explicit_paging

lib/generator.js Outdated

if ((config.archive_generator.overwrite_latest || false) && pages.length > 0) {
const lastPage = pages[pages.length - 1];
lastPage.path = lastPage.path.replace(/\/page\/\d+\/?$/, '/latest/');
Copy link
Member

@uiolee uiolee Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this destroys the unambiguity of explicit_paging.
I think adding an additional path will be better than overwriting it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not modifying if there is a single page. The objective is to have it changed even if there is 1 page. For the users is transparent, it the flag is set then all the pages (even if there is only one) end with /latest in the path

for example /tag/mytag/1 is renamed to /tag/mytag/latest and properly linked if you create a page with all the tags.

the overwrite is made on purpose so that is not duplicating pages (the static generator create the page), as the last page (with number) is never used. You can see the result in my blog.

@uiolee
Copy link
Member

uiolee commented Jan 2, 2025

maybe moving overwrite_latest verbose to hexo-pagination will be better, I dont sure.

@azimonti
Copy link
Author

moving it to hexo-pagination is an option so it can be done once for all the plugin.
But the overwrite is made on purpose.
I have about 90 tags, having 90 duplicated \tag\mytag\1 and \tag\mytag\latest is a waste of generator time and create additional pages that are not used nor needed.

Co-authored-by: Uiolee <[email protected]>
Signed-off-by: Marco Azimonti <[email protected]>
@azimonti azimonti marked this pull request as draft January 16, 2025 09:35
Copy link
Author

@azimonti azimonti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the logic into hexo-pagination. If the implementation of hexo-pagination is fine and merged, then I can create a test case for this plugin also before the merge. Now I cannot create the fixture because hexo-pagination is not recognizing the new flag.

@azimonti azimonti marked this pull request as ready for review January 16, 2025 10:46
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.

3 participants