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

Improve DX for sidebar prop in <StarlightPage> and document it #1534

Merged
merged 8 commits into from
Feb 20, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented Feb 19, 2024

Description

This PR adds a parsing step to process the value of sidebar passed to <StarlightPage>. This allows us to improve DX by not requiring lots of boilerplate setting default values, for example:

Before
<StarlightPage
	sidebar={[
		{
			type: 'link',
			label: 'Custom link 1',
			href: '/test/1',
			isCurrent: false,
			badge: undefined,
			attrs: {},
		}
	]}
/>
After
<StarlightPage
	sidebar={[
		{ 
			label: 'Custom link 1',
			href: '/test/1',
		}
	]}
/>

Setting these extra props is still valid, but not required. type has been marked as @deprecated as it can be inferred so there is no real need to set it manually although you still can.

While working on this and the documentation, I realised we leaked a difference in naming between the internal SidebarEntry shape and the public sidebar config used in astro.config.mjs. In user config, link groups have an items array, whereas internally we named this entries. It’s probably a good idea to align these so that we have some consistency.

In this PR I’ve added support for setting items in link groups so we can document that. Setting entries instead is still also supported although it is marked as @deprecated to steer people away from it. In a future version we can start warning about use of entries, and later on remove support entirely. For now, I was aiming for full backwards compatibility so this can go out as a patch.

Aside: there’s a fair amount of fiddly Zod stuff to get this working — should get cleaner once we remove entries support.

Copy link

changeset-bot bot commented Feb 19, 2024

🦋 Changeset detected

Latest commit: 85d373b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Feb 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Feb 20, 2024 6:37pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
starlight-i18n ⬜️ Ignored (Inspect) Feb 20, 2024 6:37pm

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Feb 19, 2024
@delucis delucis added the 🌟 patch Change that triggers a patch release label Feb 19, 2024
@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Feb 19, 2024

size-limit report 📦

Path Size
/index.html 5.23 KB (0%)
/_astro/*.js 21.54 KB (0%)
/_astro/*.css 13.13 KB (0%)

@delucis delucis requested a review from HiDeoo February 19, 2024 14:14
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just a couple of quick comments from me!

docs/src/content/docs/guides/pages.mdx Outdated Show resolved Hide resolved
If not set, the page will use the default global sidebar.

For example, the following page overrides the default sidebar with a link to the homepage and a group of links to different constellations.
The current page in the sidebar is set using the `isCurrent` property and an optional `badge` has been added to a link item.
Copy link
Member

Choose a reason for hiding this comment

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

The example is really explicit! Nicely done!

I am left wondering whether I can still do things like autogenerate here, though. If so, it might be nice to show another grouping where that happens? If not, a line mentioning that this config should manually list out the entire contents of the sidebar as autogeneration of groups is not available, or something like that, would be helpful!

Copy link
Member Author

Choose a reason for hiding this comment

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

Autogeneration is not supported here no — it’s really just what is shown! I’m actually torn about documenting that we don’t support it. Discussed a bit on Discord with @HiDeoo about the idea that it could be supported in theory. So I’d kind of like to avoid explicitly saying we don‘t as a fishing hook to see if we get feedback that people need/are trying to use it.

Copy link
Member

@HiDeoo HiDeoo left a comment

Choose a reason for hiding this comment

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

Finally got the time to play more than a few minutes with the changes. In the end, pretty much of all the logic is in the Zod schema and I have to say, it works exactly like I expected after testing a few scenarios. 👍

I'm really happy with the result, way nicer API for users.

Unrelated note: I'd like at some point to see if we can slightly improve our Zod error map, specially around unions, it's not that great right now, but definitely in another PR.

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@delucis
Copy link
Member Author

delucis commented Feb 20, 2024

I'm really happy with the result, way nicer API for users.

Yay!

Unrelated note: I'd like at some point to see if we can slightly improve our Zod error map, specially around unions, it's not that great right now, but definitely in another PR.

I actually have an old work-in-progress branch that made some improvements here and this work also reminded me of that. Will try to dust it off and share in case there’s something useful there.

@delucis delucis merged commit aada680 into main Feb 20, 2024
9 checks passed
@delucis delucis deleted the dx-1028/sidebar-prop branch February 20, 2024 18:41
@astrobot-houston astrobot-houston mentioned this pull request Feb 20, 2024
log101 pushed a commit to log101/starlight that referenced this pull request Feb 23, 2024
…ithastro#1534)

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
HiDeoo added a commit to HiDeoo/starlight that referenced this pull request Feb 23, 2024
* main: (21 commits)
  i18n(ja): Update plugins.mdx (withastro#1550)
  [ci] format
  docs: Added Portuguese (pt-PT) language (withastro#1503)
  [ci] format
  i18n(fr): Update pages.mdx and plugins.mdx (withastro#1545)
  i18n(hi): update outdated pages (withastro#1539)
  i18n(es): Update getting-started.mdx (withastro#1531)
  [ci] format
  i18n(es): add "pages" page (withastro#1537)
  i18n(hi): add `customization.mdx` (withastro#1538)
  Update Vietnamese UI translations (withastro#1544)
  [ci] format
  [ci] release (withastro#1540)
  Improve DX for `sidebar` prop in `<StarlightPage>` and document it (withastro#1534)
  i18n(zh-tw): add Traditional Chinese UI translations (withastro#1504)
  Export `StarlightPageProps` type (withastro#1527)
  [ci] format
  Add new starlight-ghostcms to Community plugins list (withastro#1536)
  docs: Add more plugins & tools (withastro#1528)
  [ci] format
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package 📚 docs Documentation website changes 🌟 patch Change that triggers a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants