Fix list styling issue#3026
Conversation
🦋 Changeset detectedLatest commit: e6a0b3e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a list styling issue that occurs when the last element of a list item is a <script> tag, which can interfere with proper styling in components like .
- Added a changeset to update the '@astrojs/starlight' package with a patch
- Documented the fix for better traceability and reference in future debugging
Files not reviewed (1)
- packages/starlight/style/markdown.css: Language not supported
delucis
left a comment
There was a problem hiding this comment.
Thanks for the fix. I think the selector is good and the comment is helpful 👍
Left a comment for us to think about.
| * need to style the last non-script element (`:not(script)`) that doesn't have a subsequent | ||
| * sibling that is not a script (`:not(:has(~ :not(script)))`). | ||
| */ | ||
| :not(script):has(~ script:last-child):not(:has(~ :not(script))) |
There was a problem hiding this comment.
Noting that :has() is technically not covered by our documented browser support matrix (not supported in Chrome/Edge 88–104 and Firefox 98–120). So far we only used it once in a non-critical hover style on <details>.
I think it’s OK for us to start to drop those older browsers (they’re releases from 2023 and earlier with <0.5% usage) but we should probably update our documented list (and maybe mention that in the changeset?)
It would also be possible here to keep the existing selector with the :has() variant added in addition as a progressive enhancement I guess? But not sure if it’s worth it.
There was a problem hiding this comment.
Good call, it's already bumped in #2322 which would fix it for Firefox but not Chrome. I'll update the PR so that it's only 1 bump for both PRs 👍
There was a problem hiding this comment.
Updated our browserslist query to support both this PR and #2322 (which I'll need to update to remove the equivalent change). It's a 0.8% coverage decrease. I've also bumped the changeset to a minor version and also described the change in it (open to suggestion regarding the wording).
What is THAT? Please don't tell me this is now enabled by default on GitHub... 🙏 |
No, we just were curious, but it's manual https://docs.github.com/en/copilot/using-github-copilot/code-review/using-copilot-code-review |
|
Yeah, we were curious, but it doesn’t even understand CSS so pretty useless 😅 |
delucis
left a comment
There was a problem hiding this comment.
Looks good I think. Left a small suggestion on the changeset for your consideration.
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Description
Initially reported in Discord by Chris and Sarah, this PR fixes a list styling issue that can happen when the last child element of a list item is a script tag.
This can naturally happen in Starlight, e.g. in a list of steps where the last element of a step is the first occurence of a set of tabs in the page. The issue is visible in the Starlight documentation of the
<Steps>component:We cannot really target the built-in tabs component to fix this issue, as we could have more components that could lead to the same issue in the future, or the user could have their own component that triggers the issue.
We also cannot fix this issue if only the last child element of a list item is a script tag, as even if rare, the last n child elements could be script tags too.
This leads to quite the complex selector to try to handle all cases, I tried to comment it as much as possible to make it more understandable.