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

Fixes an edge case in custom pagination link processing #2105

Merged
merged 5 commits into from
Jul 7, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/small-kangaroos-smile.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@astrojs/starlight': patch
---

Fixes an edge case in custom pagination link processing

Custom link values for `prev`/`next` in page frontmatter are now always used as authored.
Previously this was not the case in some edge cases such as for the first and final pages in the sidebar.
2 changes: 1 addition & 1 deletion packages/starlight/__tests__/basics/navigation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ describe('getPrevNextLinks', () => {
expect(withDefaults.prev).toBeUndefined();
expect(withCustomLinks.prev).toEqual({
type: 'link',
href: '/x',
href: 'x',
label: 'X',
isCurrent: false,
attrs: {},
Expand Down
76 changes: 76 additions & 0 deletions packages/starlight/__tests__/basics/pagination-with-base.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { afterAll, beforeAll, describe, expect, test, vi } from 'vitest';
import type {
getPrevNextLinks as getPrevNextLinksType,
getSidebar as getSidebarType,
} from '../../utils/navigation';

vi.mock('astro:content', async () =>
(await import('../test-utils')).mockedAstroContent({
docs: [
['index.mdx', { title: 'Home Page' }],
['environmental-impact.md', { title: 'Eco-friendly docs' }],
['guides/authoring-content.md', { title: 'Authoring Markdown' }],
['guides/components.mdx', { title: 'Components' }],
['reference/frontmatter.md', { title: 'Frontmatter Reference' }],
],
})
);

describe('without base', async () => {
let getPrevNextLinks: typeof getPrevNextLinksType;
let getSidebar: typeof getSidebarType;
let sidebar: ReturnType<typeof getSidebar>;

beforeAll(async () => {
({ getPrevNextLinks, getSidebar } = await import('../../utils/navigation'));
sidebar = getSidebar('/reference/frontmatter/', undefined);
});

test('pagination links are formatted correctly with no frontmatter', () => {
const links = getPrevNextLinks(sidebar, true, {});
expect(links.prev?.href).toMatchInlineSnapshot(`"/guides/components/"`);
expect(links.next?.href).toMatchInlineSnapshot(`undefined`);
});

test('pagination links are formatted correctly with custom links in frontmatter', () => {
delucis marked this conversation as resolved.
Show resolved Hide resolved
const links = getPrevNextLinks(sidebar, true, {
prev: { link: '/other-page', label: 'Other Page' },
next: { link: '/extra-page', label: 'Extra Page' },
});
expect(links.prev?.href).toMatchInlineSnapshot(`"/other-page"`);
expect(links.next?.href).toMatchInlineSnapshot(`"/extra-page"`);
});
});

describe('with base', () => {
let getPrevNextLinks: typeof getPrevNextLinksType;
let getSidebar: typeof getSidebarType;
let sidebar: ReturnType<typeof getSidebar>;

beforeAll(async () => {
vi.resetModules();
vi.stubEnv('BASE_URL', '/test-base/');
({ getPrevNextLinks, getSidebar } = await import('../../utils/navigation'));
sidebar = getSidebar('/test-base/reference/frontmatter/', undefined);
});

afterAll(() => {
vi.unstubAllEnvs();
vi.resetModules();
});

test('pagination links are formatted correctly with no frontmatter', () => {
const links = getPrevNextLinks(sidebar, true, {});
expect(links.prev?.href).toMatchInlineSnapshot(`"/test-base/guides/components/"`);
expect(links.next?.href).toMatchInlineSnapshot(`undefined`);
});

test('pagination links are formatted correctly with custom links in frontmatter', () => {
const links = getPrevNextLinks(sidebar, true, {
prev: { link: '/other-page', label: 'Other Page' },
next: { link: '/extra-page', label: 'Extra Page' },
});
expect(links.prev?.href).toMatchInlineSnapshot(`"/other-page"`);
expect(links.next?.href).toMatchInlineSnapshot(`"/extra-page"`);
});
});
30 changes: 22 additions & 8 deletions packages/starlight/utils/navigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ function linkFromSidebarLinkItem(
if (locale) href = '/' + locale + href;
}
const label = pickLang(item.translations, localeToLang(locale)) || item.label;
return makeLink(href, label, currentPathname, item.badge, item.attrs);
return makeSidebarLink(href, label, currentPathname, item.badge, item.attrs);
}

/** Create a link entry from an automatic internal link item in user config. */
Expand Down Expand Up @@ -160,11 +160,11 @@ function linkFromInternalSidebarLinkItem(
}
const label =
pickLang(item.translations, localeToLang(locale)) || item.label || entry.entry.data.title;
return makeLink(entry.slug, label, currentPathname, item.badge, item.attrs);
return makeSidebarLink(entry.slug, label, currentPathname, item.badge, item.attrs);
}

/** Create a link entry. */
function makeLink(
/** Process sidebar link options to create a link entry. */
function makeSidebarLink(
href: string,
label: string,
currentPathname: string,
Expand All @@ -174,10 +174,24 @@ function makeLink(
if (!isAbsolute(href)) {
href = formatPath(href);
}

const isCurrent = pathsMatch(encodeURI(href), currentPathname);
return makeLink({ label, href, isCurrent, badge, attrs });
}

return { type: 'link', label, href, isCurrent, badge, attrs: attrs ?? {} };
/** Create a link entry */
function makeLink({
isCurrent = false,
attrs = {},
badge = undefined,
...opts
}: {
label: string;
href: string;
isCurrent?: boolean;
badge?: Badge | undefined;
attrs?: LinkHTMLAttributes | undefined;
}): Link {
return { type: 'link', ...opts, badge, isCurrent, attrs };
}

/** Test if two paths are equivalent even if formatted differently. */
Expand Down Expand Up @@ -240,7 +254,7 @@ function treeify(routes: Route[], baseDir: string): Dir {

/** Create a link entry for a given content collection entry. */
function linkFromRoute(route: Route, currentPathname: string): Link {
return makeLink(
return makeSidebarLink(
slugToPathname(route.slug),
route.entry.data.sidebar.label || route.entry.data.title,
currentPathname,
Expand Down Expand Up @@ -388,7 +402,7 @@ function applyPrevNextLinkConfig(
} else if (config.link && config.label) {
// If there is no link and the frontmatter contains both a URL and a label,
// create a new link.
return makeLink(config.link, config.label, '');
return makeLink({ href: config.link, label: config.label });
}
}
// Otherwise, if the global config is enabled, return the generated link if any.
Expand Down
Loading