Skip to content
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

add docs for meta attribute in Code component #9054

Merged
merged 8 commits into from
Aug 14, 2024

Conversation

jcayzac
Copy link
Contributor

@jcayzac jcayzac commented Aug 10, 2024

Description (required)

Documentation for withastro/astro#11605

Related issues & labels (optional)

For Astro version: 4.14.0. See astro PR 11605

First-time contributor to Astro Docs?

Discord: @jcayzac

@github-actions github-actions bot added the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Aug 10, 2024
Copy link

netlify bot commented Aug 10, 2024

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 84f8382
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/66bb9ef9ce1cd80008a80cb6
😎 Deploy Preview https://deploy-preview-9054--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@astrobot-houston
Copy link
Contributor

Hello! Thank you for opening your first PR to Astro’s Docs! 🎉

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any broken links you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Netlify 🥳.

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

@github-actions github-actions bot removed the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Aug 10, 2024
@astrobot-houston
Copy link
Contributor

astrobot-houston commented Aug 10, 2024

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale File Note
en reference/api-reference.mdx Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@jcayzac jcayzac changed the title add docs for meta atrribute in Code component add docs for meta attribute in Code component Aug 11, 2024
@Princesseuh Princesseuh added this to the 4.14 milestone Aug 12, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this along with your PR, @jcayzac !

I think we can do better than duplicating an entire huge example just to add the meta property, though. So I may decide this should just be a line in the main transformers section, and we just show one example including the meta tag. We do something similar in the config section on the Markdown page, where we just show one config example, and one of the plugins happens to have additional options passed.

I'm thinking maybe something like:

[Shiki transformers](https://shiki.style/packages/transformers#shikijs-transformers) can optionally be applied to code by passing them in through the `transformers` property as an array. Since Astro v4.14.0, you can also provide a string for [Shiki's `meta` attribute](https://shiki.style/guide/transformers#meta) to pass options to transformers.

And then update the code sample to include both transformers in the array, plus the meta option, highlighting the two lines relevant to transformers (we use Expressive Code to do that in Docs 😉)

astro title="src/pages/index.astro" {12-13}

What do you think about something like that?

src/content/docs/en/reference/api-reference.mdx Outdated Show resolved Hide resolved
@sarah11918
Copy link
Member

Oops, I see from the original PR that you're describing meta completely independent of the transformer usage. I think I need more context about how you see this feature used, as your examples are probably showing specific use cases without painting a general picture first. (Which is why I inferred incorrectly what the option was in general).

I asked for clarification in the PR there, or you can reply here!

If meta is a standalone attribute that has use outside of transformers, then it shouldn't be explained within transformers. I'll need a more general introduction to what meta is and is used for first, and then we can show examples. This will help me figure out where the documentation goes.

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@jcayzac
Copy link
Contributor Author

jcayzac commented Aug 12, 2024

I asked for clarification in the PR there,

Erika replied there.

I'm thinking maybe something like:

[Shiki transformers](https://shiki.style/packages/transformers#shikijs-transformers) can optionally be applied to code by passing them in through the `transformers` property as an array. Since Astro v4.14.0, you can also provide a string for [Shiki's `meta` attribute](https://shiki.style/guide/transformers#meta) to pass options to transformers.

And then update the code sample to include both transformers in the array, plus the meta option, highlighting the two lines relevant to transformers

LGTM!

(we use Expressive Code to do that in Docs 😉)

astro title="src/pages/index.astro" {12-13}

I did not understand that part. Expressive Code is not relevant to that documentation.

@sarah11918
Copy link
Member

Expressive Code is not a part of what we are documenting here, but it's how we have to format our code samples of your example here! I'm just pointing out that I find it coincidental that you have provided an example about line highlighting showing how to do it one way, but in order for us to actually document that, we have to use Expressive Code to do the highlighting of your example! 😄

@sarah11918
Copy link
Member

Thanks @jcayzac ! Can you please take a look over my changes and make sure they're correct? Anything else you notice or want to see different here?

@sarah11918 sarah11918 added add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah! labels Aug 12, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for docs! @jcayzac , if you spot anything before this is released, feel free to return and let me know.

Otherwise, I'll consider this PR to be ready for our minor release on Thursday! Thank you for this contribution, and welcome to Team Docs! 🥳

@sarah11918 sarah11918 added the Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! label Aug 12, 2024
@@ -2124,7 +2124,8 @@ console.log(foo + bar) // [!code focus]
<Code
code={code}
lang="js"
transformers={[transformerNotationFocus()]} />
transformers={[transformerNotationFocus(),[transformerMetaHighlight()]}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bogus square bracket here.

Suggested change
transformers={[transformerNotationFocus(),[transformerMetaHighlight()]}
transformers={[transformerNotationFocus(), transformerMetaHighlight()]}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! Oops! We'll fix this when we decide exactly which transformer(s) we should show!

@@ -2124,7 +2124,8 @@ console.log(foo + bar) // [!code focus]
<Code
code={code}
lang="js"
transformers={[transformerNotationFocus()]} />
transformers={[transformerNotationFocus(),[transformerMetaHighlight()]}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because transformerNotationFocus blurs everything but the line marked with // [!code focus], using it together with transformerMetaHighlight makes little sense IMHO.

Or at least the highlighted line should be the same as the focused line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! Shall we just delete the NotationFocus transformer then? Would also be OK if you wanted to suggest two that did go together! Just let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarah11918 sorry it took me some time to circle back to this PR. I see it's merged now.

The example now doesn't work. I has style for a transformer that's not used, and doesn't have any style for the transformer it uses.

The fixed example is that of the original commit: 975a6e1#diff-42959260ea642887b866e8e276a4520c1f112356eb9c15b31e510132a481aed6R2147-R2165

@jcayzac
Copy link
Contributor Author

jcayzac commented Aug 13, 2024

Thanks @sarah11918, I commented on the issues I found.

@sarah11918 sarah11918 changed the base branch from main to 4.14.0 August 13, 2024 17:59
@sarah11918 sarah11918 merged commit 94753c2 into withastro:4.14.0 Aug 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants