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

added renameLast and verbose options #133

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

Conversation

azimonti
Copy link

@azimonti azimonti commented Jan 16, 2025

check list

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

Description

Adding the option to have the last page to be latest which is convenient in case of reverse pagination.
Adding a verbose option to display the generated routes

Additional information

@@ -78,6 +80,16 @@ function pagination(base, posts, options = {}) {
});
}

if ((overwriteLatest && result.length > 1) || (overwriteLatest && explicitPaging && result.length === 1)) {
const lastPage = result[result.length - 1];
lastPage.path = lastPage.path.replace(/\/page\/\d+\/?$/, '/latest/');
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/hexojs/hexo-pagination?tab=readme-ov-file#usage

The url format may be diverse, and this matching method is not appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

| Data | Description |
| ------------- | ------------------------------------------------- |
| `base` | Base URL |
| `total` | Total pages |
| `current` | Current page number |
| `current_url` | Path of the current page (which equals to `path`) |
| `posts` | The slice of posts for the current page |
| `prev` | Previous page number |
| `prev_link` | The path to the previous page |
| `next` | Next page number |
| `next_link` | The path to the next page |

per page's data contains current_url prev_link next_link. Only modifying the path will cause wrong reference

README.md Outdated
| `layout` | Layout. This value can be a string or an array. | `['archive', 'index']` |
| `data` | Extra data | `{}` |
| `explicitPaging` | Number the first page. e.g. `page/1/index.html` | `false` |
| `overwriteLatest`| Set the latest page name. e.g. `page/latest/index.html` | `false` |
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if you have done any relevant testing. By default, articles are sorted in reverse order by date, so the first page is "latest".

However, different users may have different settings, so I would suggest that two options should be provided, freely choosing to overwrite the first page or the last page.

For this PR, I would recommend that call it "lastpage" instead of "latest"

test/index.js Outdated
}
result[result.length - 1].path.should.eql('/latest/');
});
it('overwriteLatest 2', () => {
Copy link
Member

Choose a reason for hiding this comment

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

It is recommended to summarize it, instead of naming it 123

@azimonti azimonti marked this pull request as draft January 22, 2025 12:44
@azimonti azimonti marked this pull request as ready for review January 22, 2025 12:45
@azimonti
Copy link
Author

thank you for the many comments and suggestions.

I made few changes:

  • simplified the logic directly within formatURL to cover all the options
  • changed the name from latest to last, but added option for user to change it as they prefer
  • added more tests (including localization and different format) and consolidate the tests in one test function

@azimonti azimonti requested a review from uiolee January 22, 2025 12:50
@azimonti azimonti changed the title added overwriteLatest and verbose options added renameLast and verbose options Jan 22, 2025
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.

2 participants