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

[i18nIgnore] Add tab component to recipes and deploy.mdx #5305

Merged
merged 16 commits into from
Nov 13, 2023
Merged

[i18nIgnore] Add tab component to recipes and deploy.mdx #5305

merged 16 commits into from
Nov 13, 2023

Conversation

jacobdalamb
Copy link
Contributor

@jacobdalamb jacobdalamb commented Nov 7, 2023

Description (required)

Add a tab component to the remaining recipe pages and deploy.mdx.

Related issues & labels (optional)

  • Closes #
  • Suggested label:

Copy link

netlify bot commented Nov 7, 2023

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b60fa61
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/65526b90baa5d60007639f5e
😎 Deploy Preview https://deploy-preview-5305--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.

@github-actions github-actions bot added the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Nov 7, 2023
@jacobdalamb jacobdalamb changed the title [i18nignore] Add tab component to remaining recipes [i18nIgnore] Add tab component to remaining recipes Nov 7, 2023
@jacobdalamb jacobdalamb changed the title [i18nIgnore] Add tab component to remaining recipes [i18nIgnore] Add tab component to recipes and deploy.mdx Nov 7, 2023
Copy link
Member

@ElianCodes ElianCodes left a 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! Did you update these for every available language (just making sure).

Also, the CI seems to fail on links / an unclosed tag, can you check that

@jacobdalamb
Copy link
Contributor Author

jacobdalamb commented Nov 7, 2023

Looks good to me! Did you update these for every available language (just making sure).

Also, the CI seems to fail on links / an unclosed tag, can you check that

As far as I can tell I updated these for every available language.

And, the CI on check links is just a fluke and should work fine if the test is rerun.

Copy link
Member

@at-the-vr at-the-vr left a comment

Choose a reason for hiding this comment

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

I ran the changes and found all of the changes in deploy.mdx files & es/recipes/external-links return a MDXError
image
I found only fix being just formatting the PackageManagerTabs Snippet similar as any of the add-yaml-support.mdx or any file that uses PackageManagerTabs (en/install/auto for example) changes

Comment on lines 44 to 62
```bash
npm install --global netlify-cli
```
<PackageManagerTabs>
<Fragment slot="npm">
```shell
npm install --global netlify-cli
```
</Fragment>
<Fragment slot="pnpm">
```shell
pnpm add --global netlify-cli
```
</Fragment>
<Fragment slot="yarn">
```shell
yarn global add netlify-cli
```
</Fragment>
</PackageManagerTabs>
Copy link
Contributor

@Genteure Genteure Nov 9, 2023

Choose a reason for hiding this comment

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

Changes involving ordered list currently are not looking right.

As an example: https://deploy-preview-5305--astro-docs-2.netlify.app/en/guides/deploy/
image

The CLI command area should be a child of the ordered list, currently it's being rendered as a sibling element between two ordered lists.
You can add indentation to make it render as a child:

-<PackageManagerTabs>
-    <Fragment slot="npm">
-      ```shell
-      npm install --global netlify-cli
-      ```
-      </Fragment>
-      <Fragment slot="pnpm">
-      ```shell
-      pnpm add --global netlify-cli
-      ```
-      </Fragment>
-      <Fragment slot="yarn">
-      ```shell
-      yarn global add netlify-cli
-      ```
-    </Fragment>
-  </PackageManagerTabs>
+    <PackageManagerTabs>
+        <Fragment slot="npm">
+          ```shell
+          npm install --global netlify-cli
+          ```
+          </Fragment>
+          <Fragment slot="pnpm">
+          ```shell
+          pnpm add --global netlify-cli
+          ```
+          </Fragment>
+          <Fragment slot="yarn">
+          ```shell
+          yarn global add netlify-cli
+          ```
+        </Fragment>
+    </PackageManagerTabs>

Copy link
Member

@at-the-vr at-the-vr Nov 9, 2023

Choose a reason for hiding this comment

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

This one is well put, I will stick with these as well. Thanks @Genteure 😄

Copy link
Member

@TheOtterlord TheOtterlord 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 the PR @jacobthesheep! 🎉

It looks like there are issues with the indenting of tabs in lists.

image

Once corrected, they should be part of the list indent.

image

I've suggested one example of correct indenting. You can view the indenting in the Netlify deploy preview once you've committed your changes. Once you have updated all the changes, we can take another look!

src/content/docs/de/guides/deploy.mdx Outdated Show resolved Hide resolved
@jacobdalamb
Copy link
Contributor Author

Thanks for the PR @jacobthesheep! 🎉

It looks like there are issues with the indenting of tabs in lists.

image

Once corrected, they should be part of the list indent.

image

I've suggested one example of correct indenting. You can view the indenting in the Netlify deploy preview once you've committed your changes. Once you have updated all the changes, we can take another look!

Should be fixed now!

@sarah11918
Copy link
Member

Hey @jacobthesheep ! The indenting looks good on all the pages except the "external links" one. Do you mind taking another look at that, and then I'm happy to get this one merged! 🚀

@jacobdalamb
Copy link
Contributor Author

Hey @jacobthesheep ! The indenting looks good on all the pages except the "external links" one. Do you mind taking another look at that, and then I'm happy to get this one merged! 🚀

Got it!

@sarah11918
Copy link
Member

Looking great now! Let's jump this one into the merge queue! 🚋

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.

Adding to Merge Queue

@sarah11918 sarah11918 merged commit 8b8d20c into withastro:main Nov 13, 2023
9 checks passed
@jacobdalamb jacobdalamb deleted the chore/vite-rollup-recipe branch November 14, 2023 17:14
yanthomasdev added a commit that referenced this pull request Dec 8, 2023
Update file with PR #5217 #5305 #5572

Co-authored-by: Yan Thomas <[email protected]>
ematipico pushed a commit that referenced this pull request Jan 26, 2024
Update file with PR #5217 #5305 #5572

Co-authored-by: Yan Thomas <[email protected]>
trueberryless added a commit to trueberryless/withastro-docs that referenced this pull request Nov 28, 2024
yanthomasdev added a commit that referenced this pull request Dec 17, 2024
* update add-yaml-support #4666, #5305. #5846, #8167

* translate commenst

---------

Co-authored-by: Yan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants