-
-
Notifications
You must be signed in to change notification settings - Fork 838
Fix Astro Island margins in Markdown #3340
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
Conversation
🦋 Changeset detectedLatest commit: 67ddf34 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. |
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
|
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 getting the ball rolling on this one @HiDeoo!
Do you have any more tests that you think we need for confidence? With more realistic scenarios?
In the abstract, this looks OK, but I find it a bit hard to predict what the real-world island usage will actually look like. Especially for slotted content.
I will note that given the additional styles, extracting the margin to a custom property might be a good idea. Would make it a bit easier for people to tweak spacing rather than needing to override all the various margin styles we’ll end up with.
Definitely 👍 That's what I meant in the PR body and I plan on doing that this week-end. I want to try with some random components I use in various projects and test various scenarios, and see if I can find any issues. Thanks for linking the Cloudflare example, I'll have to test against it too as I mostly worked on relatively simple components so far. I'll undraft the PR once I have done that and feel more confident about the changes. Regarding the refactor, totally agree, right now I just added new rules and did not even try to see if some smarter refactors could be done or not. |
|
Added a few more examples, but turns out after checking, I'm not a heavy island user and even less in the middle of content, most of the time it would be in layouts or just Astro components 😅 I added one matching the Cloudflare example, a few others that came to mind, but in the end, I still feel like I may not have a full grasp of all or most common use cases of islands in the middle of content. I'll try exploring something like |
|
As mentioned, I've been trying more examples based on searches like https://github.com/search?q=language%3Amdx+%22client%3Aonly%22&type=code&p=1 and a few others. So far, every examples behaved as expected using the new changes from this PR but I'm still not super confident about the change considering I'm not a heavy island user, even more in the middle of content. I'm opening up this PR for review in case it can bring more awareness to the change and get more feedback from the community if some users are running into similar setups and can to test it out. I will only move forward with the suggested refactoring if we decide to ship this change. |
delucis
left a comment
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 personally pretty happy with the proposal and the thoroughness of the testing we’ve done — after all the PR is currently 3 lines of genuine CSS additions, and more than 1,000 lines of testing 😁
I think we could release it and see if we get more feedback.
Do you think it should be a minor given the changes? Or do we think it’s safe enough for a patch? People will either get improved spacing OR if they already did something to remediate it nothing should change (their custom CSS wins or they have wrapper elements which are not impacted by this change).
I'm a bit afraid of the opposite case where someone was relying on the previous behavior for some reason and do not want the margin, so I'm leaning towards a minor and including in the changelog some code to get back the previous behavior. |
Sure — gives us a bit more space to document the change like you say 👍 |
* main: (55 commits) i18n(de): translate `themes.mdx` (withastro#3397) [ci] format i18n(ko): translate `themes.mdx` (withastro#3398) [ci] format docs: add Starlight Page to themes showcase (withastro#3393) i18n(de): update German themes (withastro#3394) docs: change image of Starlight Galaxy Theme dark image to not include a visible scrollbar (withastro#3395) [ci] format i18n(ko-KR): update `themes.mdx` (withastro#3392) [ci] format Add link to new Starlight theme (withastro#3390) i18n(fr): update `guides/css-and-tailwind.mdx` (withastro#3389) i18n(de): translate `guides/css-and-tailwind.mdx` (withastro#3386) [ci] format i18n(ko-KR): update `css-and-tailwind.mdx` (withastro#3388) Adds OmniPrint documentation to the showcase (withastro#3387) docs(showcase): Add Aptos Docs (withastro#3385) docs: add section about multiple Tailwind configs (withastro#3273) Add basic ESLint config, lint script and lint CI job (withastro#1640) chore(deps): update actions/checkout action to v5 (withastro#3375) ...
|
Updated and cleaned up the PR. I introduced a new CSS custom property as suggested (open on suggestions regarding the naming, e.g. what it represents vs. what it does). I also decided against merging the 2 rules into one, I think it hurts readability too much. |
delucis
left a comment
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.
Looks good to me — and the variable naming seems spot on 🎯
|
Thanks HiDeoo for the awesome update 🙌 |
Since we re-use styles from Starlight, we need to set the new `--sl-content-gap-y` variable. See this PR for more context: withastro/starlight#3340
* Bump the npm group with 5 updates Bumps the npm group with 5 updates: | Package | From | To | | --- | --- | --- | | [@astrojs/sitemap](https://github.com/withastro/astro/tree/HEAD/packages/integrations/sitemap) | `3.5.1` | `3.6.0` | | [@astrojs/starlight](https://github.com/withastro/starlight/tree/HEAD/packages/starlight) | `0.35.2` | `0.36.0` | | [astro](https://github.com/withastro/astro/tree/HEAD/packages/astro) | `5.13.7` | `5.13.9` | | [sharp](https://github.com/lovell/sharp) | `0.34.3` | `0.34.4` | | [@tailwindcss/typography](https://github.com/tailwindlabs/tailwindcss-typography) | `0.5.16` | `0.5.18` | Updates `@astrojs/sitemap` from 3.5.1 to 3.6.0 - [Release notes](https://github.com/withastro/astro/releases) - [Changelog](https://github.com/withastro/astro/blob/main/packages/integrations/sitemap/CHANGELOG.md) - [Commits](https://github.com/withastro/astro/commits/@astrojs/[email protected]/packages/integrations/sitemap) Updates `@astrojs/starlight` from 0.35.2 to 0.36.0 - [Release notes](https://github.com/withastro/starlight/releases) - [Changelog](https://github.com/withastro/starlight/blob/main/packages/starlight/CHANGELOG.md) - [Commits](https://github.com/withastro/starlight/commits/@astrojs/[email protected]/packages/starlight) Updates `astro` from 5.13.7 to 5.13.9 - [Release notes](https://github.com/withastro/astro/releases) - [Changelog](https://github.com/withastro/astro/blob/main/packages/astro/CHANGELOG.md) - [Commits](https://github.com/withastro/astro/commits/[email protected]/packages/astro) Updates `sharp` from 0.34.3 to 0.34.4 - [Release notes](https://github.com/lovell/sharp/releases) - [Commits](lovell/sharp@v0.34.3...v0.34.4) Updates `@tailwindcss/typography` from 0.5.16 to 0.5.18 - [Release notes](https://github.com/tailwindlabs/tailwindcss-typography/releases) - [Changelog](https://github.com/tailwindlabs/tailwindcss-typography/blob/main/CHANGELOG.md) - [Commits](tailwindlabs/tailwindcss-typography@v0.5.16...v0.5.18) --- updated-dependencies: - dependency-name: "@astrojs/sitemap" dependency-version: 3.6.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: npm - dependency-name: "@astrojs/starlight" dependency-version: 0.36.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: npm - dependency-name: astro dependency-version: 5.13.9 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm - dependency-name: sharp dependency-version: 0.34.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm - dependency-name: "@tailwindcss/typography" dependency-version: 0.5.18 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm ... Signed-off-by: dependabot[bot] <[email protected]> * Fix content margin for non-Starlight pages Since we re-use styles from Starlight, we need to set the new `--sl-content-gap-y` variable. See this PR for more context: withastro/starlight#3340 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Kyle Lacy <[email protected]>
* main: (26 commits) 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) [i18nIgnore] Fix duplicate i18n collection definition (withastro#3409) ...
* 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) ...
* main: (87 commits) i18n(ja): sync outdated aside page (withastro#3465) i18n(ja): sync outdated icon reference (withastro#3467) chore(deps): update pnpm/action-setup action to v4.2.0 (withastro#3460) 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) ...

Description
This PR attempts to fix an issue where Starlight's Markdown default margins are not applied when using interactive UI frameworks components (Astro Islands).
I don't think we can really reuse our existing rule for this and we may need to have a special case targetting the first child of an island. This PR temporarily adds a few test scenarios to the
/getting-started/page and I'll also play around with more to see if there are other edge cases we might need to consider.