Move <summary> to top of <details> for validity#3423
Conversation
🦋 Changeset detectedLatest commit: a127aac 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. |
|
I’m just trying to get my Starlight output to pass an HTML validator. (Example.) Of course I’m aware that not all validation errors are problems in the real world, so I’m filtering out a mix of validator bugs, legitimate nonstandard extensions, and browser bug workarounds to find the real issues. This case clearly does violate the standard, so even if it doesn’t cause problems in practice with today’s common browsers, we might as well fix it to make it easier for real issues to stand out. |
delucis
left a comment
There was a problem hiding this comment.
Thanks for the extra context @andersk!
I was hesitant just because the sidebar restoration logic here is a little subtle, but I tested it again and I think this change is safe.
Ideally, we do want the restore point as early as possible in the DOM, so I’ve suggested an alternative refactor. What do you think? Both work in practice, just thinking what’s the most minimal change in terms of execution order (the <SidebarRestorePoint> component renders a custom element which controls the collapsed state of the parent <details>)
| > | ||
| <SidebarRestorePoint /> | ||
| <summary> | ||
| <div class="group-label"> |
There was a problem hiding this comment.
I wonder if another option here would be to move the restore point inside the <summary> so it’s still as early as possible in the tree:
| <div class="group-label"> | |
| <SidebarRestorePoint /> | |
| <div class="group-label"> |
| </div> | ||
| <Icon name="right-caret" class="caret" size="1.25rem" /> | ||
| </summary> | ||
| <SidebarRestorePoint /> |
There was a problem hiding this comment.
And then remove it here:
| <SidebarRestorePoint /> |
9a2ab1f to
a18d8c5
Compare
|
Yeah that seems fine to me: |
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
a18d8c5 to
d7964a9
Compare
* 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) ...


The
<summary>element must be the first child of<details>.