-
-
Notifications
You must be signed in to change notification settings - Fork 829
Ensure CSS layer order in custom pages #3351
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
base: main
Are you sure you want to change the base?
Ensure CSS layer order in custom pages #3351
Conversation
🦋 Changeset detectedLatest commit: 2c358be 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 project configuration. |
|
Took the time to test the approach on more projects and it seems to work well. Before continuing with this PR, I'm undrafting it to get some feedback on the approach. While this kind of approach could usually be considered a code smell, or a sign of a design flaw, in this specific case and due to how import order for stylesheets works in Astro and how it can impact layer order, I'm failing to find other ways to achieve the desired outcome consistently. I'm hoping to get some feedback on the approach and whether it seems reasonable in this specific context or if we should continue to rely on documentation to guide users to the correct order of imports when using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally had a chance to think through the change and also chatted about it a bit with some of the Astro core team to see if there were any ideas, but I think this is looking like our best bet — thanks for the thorough and thoughtful implementation.
There’s of course part of me that is not the happiest with needing a Vite plugin that messes with the order of things, as it definitely feels like a code smell, but every other approach I can imagine would have edge cases where it didn’t work. I also considered whether could avoid the need for magic-string, but I think it may be worth adding just for the source map support.
One suggestion that came up could be detecting the import not being first and erroring and asking users to fix it themselves. But I think in that case, we’d basically still need most of this logic, so we might as well fix it rather than forcing people to do surprising things in their code.
One thought I had, which you can consider, is that rather than moving the user’s import, we could in theory prepend a side-effect import, i.e. make the plugin do this:
+ import '@astrojs/starlight/components/StarlightPage.astro';
import 'foo.css';
import Starlight Page from '@astrojs/starlight/components/StarlightPage.astro';
// ...Not sure if that could potentially be simple enough to compute the source map for that we could avoid the need for magic-string.
|
Yeah, this is definitely a tricky case. I also spent a lot of time thinking and trying out various approaches as like I mentioned, I'm also not super happy with this one but this is one of those cases where I think that the trade-offs may be needed as there's no perfect solution afaik (or even a good one providing a good DX).
Interesting idea, I haven't thought about that. I cannot yet answer your last question as this is not something I've tried, but I'll explore the idea once I'm done catching up with Astro/Starlight and plugins. I'll report back here once I have more info. |
* main: (72 commits) Showcase: Add Saucer (withastro#3454) i18n(ja): resources/themes (withastro#3442) add openstatus showcase (withastro#3436) [ci] release (withastro#3434) Add `head` config validation for `meta` tags with `content` (withastro#3380) Fix Astro Island margins in Markdown (withastro#3340) Ensure tab links span the full tab height (withastro#3427) Move `<summary>` to top of `<details>` for validity (withastro#3423) Run all tests on CI workflow changes (withastro#3433) [ci] format i18n(fr): small rewording and fixes in top-level pages (withastro#3432) i18n(fr): small rewording and fixes in `guides` (withastro#3431) i18n(fr): fix typo and links in `reference` files (withastro#3429) i18n(fr): small rewording in `components` (withastro#3430) [ci] release (withastro#3384) i18n(de): Update `page.lastUpdated` translation to be more in‐line with the English translation (withastro#3416) Remove invalid value attribute from `<select>` (withastro#3421) Convert invalid `<div>` child of `<summary>` to `<span>` (withastro#3422) [i18nIgnore] Fix typo in Spanish docs documentation (withastro#3408) chore(deps): update actions/labeler action to v6.0.1 (withastro#3407) ...
Great idea, works perfectly and slightly simplifies the code too.
This is tricky. As we transform source code, we have to generate a sourcemap. The plugin only have to return the sourcemap of the tranformations done in that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for investigating that! astro already includes magic-string so it doesn’t really hurt too much for us to use it as well, although we should make sure we track their version ranges so that it gets reused (looks like this is fine at the moment).
Left a few small comments where I spotted things, but I think this is looking pretty great! If you want to go ahead cleaning things up, I think you probably can 🙌
| import type { ViteUserConfig } from 'astro'; | ||
| import MagicString from 'magic-string'; | ||
|
|
||
| const starlightPageImportSource = '@astrojs/starlight/components/StarlightPage.astro'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m thinking to myself this will break if anyone ever aliases this, but never mind 😁
Co-authored-by: delucis <[email protected]>
Co-authored-by: delucis <[email protected]>
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
Thanks for the review, the PR should now be updated without any of the demo content, and I also added a changeset and reverted #3199. |
Co-authored-by: Felix Schneider <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really great! I left some thoughts on testing, but otherwise LGTM!
packages/starlight/__e2e__/fixtures/basics/src/pages/starlight-page-css-layer-order.astro
Show resolved
Hide resolved
| test.describe('css layer order', () => { | ||
| test('ensures that the StarlightPage component is always imported first to ensure a predictable CSS layer order in custom pages', async ({ | ||
| page, | ||
| makeServer, | ||
| }) => { | ||
| const starlight = await makeServer('dev', { mode: 'dev' }); | ||
| await starlight.goto('/starlight-page-css-layer-order'); | ||
|
|
||
| const linkButton = page.getByRole('link', { name: 'Tabs link button' }); | ||
| await linkButton.highlight(); | ||
|
|
||
| const [bgColor, textColor] = await linkButton.evaluate((element) => { | ||
| const styles = window.getComputedStyle(element); | ||
| return [styles.getPropertyValue('background-color'), styles.getPropertyValue('color')]; | ||
| }); | ||
|
|
||
| // If the background color and text color are the same, the text is not readable and the CSS | ||
| // layer order is incorrect. | ||
| expect(bgColor).not.toBe(textColor); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test feels a little fragile. Do you think we should maybe test something a bit more explicit? Like a custom user style that could end up overridden or something? Or do you think the behavior here is predictable enough that the test is unlikely to keep passing “by accident”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten that test to make it more about the layer order itself rather than side-effects of good or bad layer order. Let me know what you think of this version.
Co-authored-by: Chris Swithinbank <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change! Yeah, I think this feels a bit more reliable. Might still be fragile in some circumstances (major Astro/Vite updates e.g.), but feels like it would be fragile in the sense that it might fail incorrectly, which is not too bad, better than passing incorrectly in this case, I think.
Great work! Should be good to go for the next minor IMO 🤩

Description
This PR aims to fix the issue 2 in #3162 and is a follow-up to #3150.
This PR experiments (this is why this PR is a draft) with a Vite plugin that ensures in custom pages that the
<StarlightPage>component is always imported first, which is necessary to ensure the correct CSS layer order due to how import order for stylesheets works in Astro.A few test pages have been added to the documentation which should all result in the same visual appearance (ignoring the last
<aside>element). Without the plugin, some differences can be observed due to different CSS layer orders between all the pages, in the same mode (dev/build) or between dev and build modes.With the plugin, all pages should look the same no matter the mode. Disabling the new plugin and running
pnpm testin thedocs/directory should also result in errors with some a11y issues due to the different layer orders.Note: unit tests are failing due to coverage thresholds atm.
Remaining tasks
If we end up moving forward with this PR, the following tasks will need to be completed: