Repository navigation
Conversation
Using the `"attribute"` scoping strategy (the default), Astro does not currently pass down a parent scoping attribute like it does with the `"where"` strategy’s class name. Unclear if that’s intentional on Astro’s part or not but instead we have to use a parent selector + `:global()` for the child instead.
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
📚 docs
size-limit report 📦
|
|
Just went through this again and marked it ready for review. I think it should be a reasonably safe change thanks to
On this point I still don’t have an answer and it may be OK? AFAICT the increase in specificity of switching to the default strategy was riskier than the decrease of using Any approach to asserting this in tests would presumably mean running something like starlight-visual-diff in CI and that feels like a pretty big hammer for something that very often won’t apply (a small % of PRs touch styling). So maybe it’s fine to leave that as a manual thing to bear in mind. |
HiDeoo
left a comment
HiDeoo
left a comment
There was a problem hiding this comment.
Thanks for the pull request 🙌
It definitely took me a while to review it as I spent a lot of time thinking about it, testing things out, etc.
Not gonna lie, this is a scary change. Not at the Starlight level (I think we can manage it, even tho the PR proves it's tricky and super easy to miss tiny issues) but more at the scale of the entire ecosystem (plugins, themes, components) and user components. Styles applied to child components using a class prop would now silently stop matching unless the child forward the data-astro-cid-* attribute (and scoped styles get a higher specificity as you mentioned). It's not something that has obvious errors, can be very subtle/easy to miss and we can't really tell people to use a tool like starlight-visual-diff on all their pages.
It could be less disruptive to continue setting
scopedStyleStrategyfor users in the cases where they do not set it themselves.
After initially reading the PR and starting to review it, this was one of the points that kinda reassured me about the approach and that I wanted to bring up. Altho, after thinking more about it, I don't think this is something we can do as scopedStyleStrategy defaults to attribute and we would have no way to differentiate when it was set by the user or not (unless Astro starts exposing userConfig or something like that).
I'm posting this review to share my current thoughts and the issues I spotted so far, but I'm still a bit torn on the overall change.
There was a problem hiding this comment.
I'm not sure we can keep the current approach. Like you said, one of the most common issues you hit during the PR was icons no longer receiving the parent scope with the "attribute" strategy, but this also means that every user would also hit the same issue.
Let's say a user has the following code:
---
import { Icon } from '@astrojs/starlight/components';
---
<Icon name="star" size="10rem" class="my-icon" />
<style>
.my-icon {
color: red;
}
</style>This would no longer work and they would need to switch to :global(.my-icon) which is a worse DX imo (we would need to document it too). In this example, without a container to scope the selector, the style would also apply to any .my-icon element on the page.
I think we would need to pass the data-astro-cid-* attribute as documented:
<svg
{...a11yAttrs}
- class:list={['sl-icon', Astro.props.class]}
+ {...attrs}
+ class:list={['sl-icon', className]}
style={{ '--sl-icon-color': color, '--sl-icon-size': size }}
width="16"
height="16"
viewBox="0 0 24 24"
fill="currentColor"
set:html={Icons[name]}
/>With this approach, I think the previous selectors would work again with all strategies, which also means we could basically revert a lot of :global() changes in the PR.
There was a problem hiding this comment.
While browsing many pages, I manually spotted a difference in the link card icon color.
| Before | After |
|---|---|
 |
 |
I checked using starlight-visual-diff but this difference was not reported. After investigating, it turns out it's an issue in starlight-visual-diff with very subtle color differences. I have now updated the tool to better detect such changes, and this page (and other pages containing link cards) are now properly reported as expected.
Regarding the issue, it's probably due to the .icon selectors not being updated to use :global().
Note
This would be fixed automatically with the approach suggested for the <Icon> component.
| } | ||
|
|
||
| :global([dir='rtl']) .caret { | ||
| :global([dir='rtl']) :global(.caret) { |
There was a problem hiding this comment.
| :global([dir='rtl']) :global(.caret) { | |
| :global([dir='rtl']) summary :global(.caret) { |
I think we need to scope this selector to avoid targeting every .caret on RTL pages.
Note
This would be fixed automatically with the approach suggested for the <Icon> component.
| align-items: center; | ||
| } | ||
| .card .icon { | ||
| .card :global(.icon) { |
There was a problem hiding this comment.
I guess this would style any .icon in a card, even in user content. This could even happen with a <LinkCard> in a <Card>.
We probably need to use something like .title > :global(.icon).
Note
This would be fixed automatically with the approach suggested for the <Icon> component.
|
|
||
| ⚠️ **BREAKING CHANGE:** Adds support for any Astro [`scopedStyleStrategy`](https://docs.astro.build/en/reference/configuration-reference/#scopedstylestrategy) configuration. | ||
|
|
||
| Previously, Starlight forced the `"where"` strategy, resulting in CSS selectors like `:where(astro-HASH)`. Starlight now respects the strategy set in `astro.config.mjs` and will default to Astro’s default `"attribute"` strategy, resulting in attribute selectors like `[data-astro-cid-HASH]`. |
There was a problem hiding this comment.
Tiny fix, also if we keep the default Astro strategy, I wonder if we should also link to https://docs.astro.build/en/guides/styling/#passing-a-class-to-a-child-component as this may be required for some of their components which is a new behavior?
Background
scopedStyleStrategy: "where". At the time, the higher specificity of the"attribute"default scoping strategy caused a lot of issues so we avoided it by sticking with the Astro v2 default of"where".@layerstyles making support for different scoping strategies more viable.This PR
scopedStyleStrategyand leave it to user discretion. This means by default users will switch to Astro’s default of"attribute"."attribute"strategy, so needed to be updated to use a parent selector and:global().starlight.componentslayer, I had to extract the<Icon>styles and move them to a global.sl-iconclass name instead of just depending on an Astro-scopedsvgelement selector. I also added this class name to the icons generated by Markdown asides for consistency.Open questions
scopedStyleStrategyfor users in the cases where they do not set it themselves. It would make Starlight’s default different from Astro though, so for now I went with removing it and documenting how to restore it in the changeset.