Repository navigation
Refactor Sätteri RTL code support plugin - #4114
Conversation
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 275e454 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 |
size-limit report 📦
|
HiDeoo
left a comment
HiDeoo
left a comment
There was a problem hiding this comment.
However, in practice, I think this covers all the scenarios we were intending?
I guess so and something like <pre><span><code> or similar is probably not common but that makes it a different behavior between the Unified and Sätteri implementation of the same plugin. Would it work to walk ctx.parent() until reaching <pre> or the root?
Co-Authored-By: HiDeoo <494699+HiDeoo@users.noreply.github.com>
I think so? I assume that’s more expensive because you have to serialize the entire parent node tree. I asked Erika on Discord for her reaction.
Yeah, true. We could update the Unified implementation to match Sätteri instead if equivalence is important? That might be cheaper than adding a lot of recursion for “correctness” even though the use cases it covers are basically all unintended. Edit: Erika said this kind of check will be cheap so I went ahead and pushed 275e454 which walks up the full tree. (For sanity, I also ported all the changes to the #4113 branch to double check it still fixes the test failure there and I confirmed it does.) |
HiDeoo
left a comment
HiDeoo
left a comment
There was a problem hiding this comment.
This looks perfect to me 🚀
Erika said this kind of check will be cheap so I went ahead and pushed 275e454 which walks up the full tree. (For sanity, I also ported all the changes to the #4113 branch to double check it still fixes the test failure there and I confirmed it does.)
Amazing, thanks for investigating and updating the PR, definitely feels better imo to have both implementations having the same behavior 🙌
* main: (219 commits) i18n(fr): update `authoring-content.mdx`, `i18n.mdx` & `plugins.md` (withastro#4128) i18n(de): update links to Astro docs (withastro#4126) Update `size-limit` config (withastro#4122) Replace 3 outdated links to Astro Docs (withastro#4120) [ci] release (withastro#4115) Update dependencies (withastro#4113) Add Astro LilyPond to Starlight showcase (withastro#4116) Refactor Sätteri RTL code support plugin (withastro#4114) Use pinned version of `pkg-pr-new` for preview releases (withastro#4111) chore(deps): update vite and rolldown (withastro#4108) i18n(ru): update translations (withastro#4107) [ci] release (withastro#4106) Add Forgejo icon (withastro#4102) [ci] format i18n(de): update `themes.mdx` (withastro#4104) i18n(de): update `plugins.mdx` (withastro#4103) i18n(de): update `community-content.mdx` (withastro#4101) [ci] release (withastro#4097) Fix union schema `extend` regression (withastro#4095) [ci] release (withastro#4090) ...
3 participants
Description
ctx.parent()we can use to get parents so we can remove the more convoluted code for working this out. Strictly speaking these do not work 1:1 — before, we’d skip applying attributes to a<code>element even if it was deeply nested in a<pre>whereas now we only skip if the<code>is a direct child of the<pre>. However, in practice, I think this covers all the scenarios we were intending?