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

Clarify how to override default components that use multiple slots. #2386

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jwoyo
Copy link

@jwoyo jwoyo commented Sep 20, 2024

The documentation lacked an explanation of how to override default components that use named slots. Passing all slots through is necessary. Omitting this step breaks the layout for example when user conditionally changes PageFrame.astro.

The commit adds an explanation and a code example.

---
// this could be: src/components/ConditionalPageFrame.astro
import type { Props } from '@astrojs/starlight/props';
import Default from '@astrojs/starlight/components/PageFrame.astro';
const isHomepage = Astro.props.slug === '';
---
{
	isHomepage ? (
		<section>My great landing page 🌟</section>
	) : (
		<Default {...Astro.props}>
			<slot />
			<Fragment slot="header"><slot name="header"/></Fragment>
			<Fragment slot="sidebar"><slot name="sidebar"/></Fragment>
		</Default>
	)
}

The documentation lacked an explanation of how to override default components that use named slots. Passing all slots through is necessary. Omitting this step breaks the layout for example when user conditionally changes PageFrame.astro.

The commit adds an explanation and a code example.
Copy link

changeset-bot bot commented Sep 20, 2024

⚠️ No Changeset found

Latest commit: 180a9b2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added the 📚 docs Documentation website changes label Sep 20, 2024
Copy link

netlify bot commented Sep 20, 2024

Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit 180a9b2
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/670f8224589a090008bf12d8
😎 Deploy Preview https://deploy-preview-2386--astro-starlight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@astrobot-houston
Copy link
Collaborator

Hello! Thank you for opening your first PR to Starlight! ✨

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any issues you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Netlify 🤩

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Sep 20, 2024

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale File Note
en guides/overriding-components.mdx Source changed, localizations will be marked as outdated.
en reference/overrides.md Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@HiDeoo
Copy link
Member

HiDeoo commented Oct 1, 2024

Thanks for your contribution 🙌

This is definitely something that we do not explicitly document and has come up a few times already. I'd like to get other people's opinion on this but my initial thought would be that this would better fit in the "Reuse a built-in component" section rather than the "Use page data" one. I'm also not sure we would need an entire new code example for this as this would be mostly identical to other ones we already have but we could link to the relevant Astro Docs section.

I think my idea would be to add a third bullet point to the "Reuse a built-in component" unordered list that could look something like this:

  • If the built-in component you are reusing also contains named slots, e.g. like the TwoColumnContent component which uses a right-sidebar slot, you need to transfer theses as well using the <slot name="right-sidebar" slot="right-sidebar" /> syntax.

Another possibility that comes to mind is to reference additional named slots in the "Overrides Reference" page for the few components that have them. At the moment, only TwoColumnContent and PageFrame have named slots.

For example, below the "Default component: TwoColumnContent.astro" line, it could say:

Default component: TwoColumnContent.astro
Named slots: right-sidebar

Like I said, I'd like to get your and other people's opinion on this before making any changes. Let me know what you think.

@jwoyo
Copy link
Author

jwoyo commented Oct 1, 2024

Thanks @HiDeoo for reviewing the proposed change. I agree that the information fits better as a third bullet point under Reuse a built-in component. Also referencing the named slots in the Overrides Reference makes sense to me as most users probably won't check in the overridden file in depth and would appreciate a hint that they need to handle the slots.

I'll wait for others' opinions as well and proceed with your suggested change in this PR if I don't hear otherwise.

@jwoyo jwoyo closed this Oct 16, 2024
@jwoyo jwoyo deleted the patch-1 branch October 16, 2024 08:29
@jwoyo jwoyo restored the patch-1 branch October 16, 2024 08:29
@jwoyo jwoyo reopened this Oct 16, 2024
@jwoyo
Copy link
Author

jwoyo commented Oct 16, 2024

Hey @HiDeoo. As I haven't heard any other opinions on that I've made the suggested changes:

  • Removed the additional code example
  • Added a third bullet point containing the code example with TwoColumnContent
  • Added the named slots: line to the Override References

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 docs Documentation website changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants