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] Remove Vue TypeScript troubleshooting section #2276

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jan 3, 2023

What kind of changes does this PR include?

  • New or updated content

Description

Reflects the upcoming breaking change in withastro/astro#5724. The troubleshooting section isn't an issue anymore as it has been fixed by Volar.

I also removed it for the translated docs.

@netlify
Copy link

netlify bot commented Jan 3, 2023

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit 13c26c5
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/63b4447dd29aa30008c5fc5f
😎 Deploy Preview https://deploy-preview-2276--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 settings.

@github-actions github-actions bot added the i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! label Jan 3, 2023
@sarah11918 sarah11918 added merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) improve documentation Enhance existing documentation (e.g. add an example, improve description) and removed i18n Anything to do with internationalization & translation efforts - ask @YanThomas for help! labels Jan 3, 2023
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks @bluwy! Going to “request changes” just to make some red UI scare us away from merging accidentally before v2 is released. No actual changes needed.

Will also add an en-only keyword to the PR name — this means our i18n tracker (#438) will ignore this commit and leave the translations in their current “needs updating” state instead of marking them as up-to-date. (Another case where I wish we had a i18nIgnore keyword instead!)

@delucis delucis changed the title Remove Vue TypeScript troubleshooting section Remove Vue TypeScript troubleshooting section (en-only) Jan 3, 2023
@sarah11918
Copy link
Member

@delucis I think this can be merged now? Upstream PR is merged and released.

@delucis
Copy link
Member

delucis commented Jan 4, 2023

Hmm. Interesting question. At the moment most users will still be on a 1.x version, so I wonder in this case if it makes sense to only remove once we actually release 2.0 to stable not just beta.

General principle I’m thinking:

  • For new features available in v2 beta: merge early and flag they require v2
  • For general housekeeping like this where our advice is currently valid, I’d lean towards merging late once v2 proper is out.

What do you think?

@delucis delucis changed the title Remove Vue TypeScript troubleshooting section (en-only) [i18nIgnore] Remove Vue TypeScript troubleshooting section Jan 4, 2023
@sarah11918
Copy link
Member

Makes sense! And, if a problem simply isn't going to come up in v2, then there's less danger having "advice for when this happens" there, because no one will read it! ;)

We could update (rather than remove the thing entirely) with a note saying that upgrading to v2 (even beta) is a solution, as this problem is fixed in v2?

@bluwy
Copy link
Member Author

bluwy commented Jan 4, 2023

I think the only thing tying this change is that Astro 1.0 would have a hardcoded link to this section of the docs. In Astro 2.0, the link is removed so the docs can be removed as well. The fix for the section is to upgrade Volar (VSCode extension), and given most extension auto-updates, it could also be safe to remove this section early if needed 🤔

@delucis
Copy link
Member

delucis commented Jan 5, 2023

So currently if a user runs into this, they would be shown a link to this section of the docs? That would make me lean towards remove later rather than earlier.

Unless you’re saying that most people will already have upgraded Volar so this error is probably already rare?

@bluwy
Copy link
Member Author

bluwy commented Jan 9, 2023

Ah I miss this notification. Yeah they would be shown the old link, but only if they try to integrate Vue and and a JSX framework. Plus yes that Volar should already be upgraded to latest for most users so they shouldn't have an issue with it today. So I think this is a pretty rare case.

@delucis
Copy link
Member

delucis commented Jan 9, 2023

Cool, sounds reasonably safe then to merge when you want.

@bluwy bluwy merged commit 72b72d4 into main Jan 10, 2023
@bluwy bluwy deleted the vue-types-update branch January 10, 2023 07:27
@sarah11918 sarah11918 added the minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah! label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve documentation Enhance existing documentation (e.g. add an example, improve description) 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.

3 participants