-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
feat(blog): add blog pageBasePath plugin option #9838
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
I did not find a way to test xxxBasePath definitions, to inspire from. :-( Please advise on how to proceed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although tests are failing and should be updated.
You can test your feature in this existing test suite.
Note that it would be nice to also ensure that the code is not sensitive to the presence/absence of a path leading/trailing slash here
describe('paginateBlogPosts', () => {
it('generates right pages', () => {
const blogPosts = [
{id: 'post1', metadata: {}, content: 'Foo 1'},
{id: 'post2', metadata: {}, content: 'Foo 2'},
{id: 'post3', metadata: {}, content: 'Foo 3'},
{id: 'post4', metadata: {}, content: 'Foo 4'},
{id: 'post5', metadata: {}, content: 'Foo 5'},
] as BlogPost[];
expect(
paginateBlogPosts({
blogPosts,
basePageUrl: '/blog',
blogTitle: 'Blog Title',
blogDescription: 'Blog Description',
postsPerPageOption: 2,
}),
).toMatchSnapshot();
expect(
paginateBlogPosts({
blogPosts,
basePageUrl: '/',
blogTitle: 'Blog Title',
blogDescription: 'Blog Description',
postsPerPageOption: 2,
}),
).toMatchSnapshot();
expect(
paginateBlogPosts({
blogPosts,
basePageUrl: '/',
blogTitle: 'Blog Title',
blogDescription: 'Blog Description',
postsPerPageOption: 10,
}),
).toMatchSnapshot();
});
});
It currently fails because you didn't add the extra method param to that test suite.
Not relate to this PR but I noticed a useless param returned in the loaded content, I'd appreciate if you could remove it: blogTagsPaginated: []
:
https://github.com/cronica-it/docusaurus-fork/blob/issue-9828/packages/docusaurus-plugin-content-blog/src/index.ts#L125
I guess I addressed all requested changes and the tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 I would add an extra test to paginateBlogPosts
to actually cover this new feature
Thanks 👍 LGTM |
Thank you, Sébastien! |
Pre-flight checklist
Motivation
In Docusaurus, the URL parts used to compose the paths are configurable via options like
routeBasePath
,tagsBasePath
, etc, but in the path used for multi-page lists, a part of the URL is hard-coded aspage
.Test Plan
TBD
Test links
TBD
Related issues/PRs
fix #9828