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

"Clarify the 'Existing Projects' section of the TypeScript docs: #52944

Merged
merged 12 commits into from Jul 21, 2023
Merged

"Clarify the 'Existing Projects' section of the TypeScript docs: #52944

merged 12 commits into from Jul 21, 2023

Conversation

ShaunFerris
Copy link
Contributor

Add a sentence to the instructions for using typescript in an existing project instructing the user to copy the paths compiler option from the existing jsconfig file to the new tsconfig file.
Not doing so causes absolute imports from project directories to break, and gives "Module Not Found" messages that the docs do not have a case for solving"

> Add a sentence to the instructions for using typescript in an existing project instructing the user to copy the `paths` compiler option from the existing jsconfig file to the new tsconfig file.
> Not doing so causes absolute imports from project directories to break, and gives "Module Not Found" messages that the docs do not have a case for solving"
@ijjk
Copy link
Member

ijjk commented Jul 20, 2023

Allow CI Workflow Run

  • approve CI run for commit: 396c2cb

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@ijjk
Copy link
Member

ijjk commented Jul 20, 2023

Allow CI Workflow Run

  • approve CI run for commit: 80e752e

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

…escript.mdx

Co-authored-by: Steven <steven@ceriously.com>
@ShaunFerris ShaunFerris requested a review from styfle July 20, 2023 15:42
@styfle styfle added the CI approved Approve running CI for fork label Jul 20, 2023
@styfle styfle removed the CI approved Approve running CI for fork label Jul 20, 2023
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Please fix the lint error

@ShaunFerris
Copy link
Contributor Author

Please fix the lint error

Hi,
I have run the linting script this time so hopefully the check will not fail. It may be worth linking to the contributing documentation from the Docs Contribution Guide, as there is currently no mention of getting the dev environment setup or running the lint script there.

@ShaunFerris ShaunFerris requested a review from styfle July 21, 2023 08:55
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks!

@ShaunFerris
Copy link
Contributor Author

ShaunFerris commented Jul 21, 2023

If any maintainer could offer advice on why these checks might be failing, particularly the Validate Doc Links check, I would appreciate it. I apologize if I'm missing something obvious, Cheers!

@styfle styfle added the CI approved Approve running CI for fork label Jul 21, 2023
@styfle
Copy link
Member

styfle commented Jul 21, 2023

GitHub is having an outage https://www.githubstatus.com/incidents/6503rcn8s34s

@styfle
Copy link
Member

styfle commented Jul 21, 2023

The "Validate Docs" was broken in a previous PR.

I think the check is actually wrong because the link looks correct. cc @delbaoliveira

@styfle
Copy link
Member

styfle commented Jul 21, 2023

It may be worth linking to the contributing documentation from the Docs Contribution Guide, as there is currently no mention of getting the dev environment setup or running the lint script there.

Thanks for pointing that out!c It would be great if you could submit another PR to improve that guide now that you have gone through the process and know what's missing. Thanks!

@ShaunFerris
Copy link
Contributor Author

It may be worth linking to the contributing documentation from the Docs Contribution Guide, as there is currently no mention of getting the dev environment setup or running the lint script there.

Thanks for pointing that out!c It would be great if you could submit another PR to improve that guide now that you have gone through the process and know what's missing. Thanks!

Cool I was thinking that I would do that, but just to clarify while I have you here, do you think the line "To contribute, you can edit the files directly on GitHub or clone the repo and edit the files locally." should be changed to remove the option of editing directly on github, as you cant run the linter from githubs editor? (Unless I am missing something).

Cheers for the feedback, I am keen to continue contributing!

@kodiakhq kodiakhq bot merged commit 3e34b9f into vercel:canary Jul 21, 2023
45 of 46 checks passed
@ShaunFerris ShaunFerris deleted the clarify-TS-docs branch July 21, 2023 15:14
@styfle
Copy link
Member

styfle commented Jul 21, 2023

as you cant run the linter from github

Maybe that's the real problem. I bet there is a way to automatically run lint in an action and commit the result.

We get 70 PRs a week so anything we can do to reduce friction would benefit everyone 👍

@ShaunFerris
Copy link
Contributor Author

as you cant run the linter from github

Maybe that's the real problem. I bet there is a way to automatically run lint in an action and commit the result.

That would be the best solve imo. I did edit locally but did not bother setting up the dev env initially as I was just editing mdx, then when I went back to lint this morning I had to spend a fair bit of time to get everything working and roll back my node to skirt around this: pnpm/pnpm#6424 issue in pnpm. Auto linting on docs edits from githubs editor would be a much lower barrier to participation.

@styfle
Copy link
Member

styfle commented Jul 21, 2023

I checked the PR template and I see that it does mention prettier fix here:

- Run `pnpm prettier-fix` to fix formatting issues before opening the PR.

I also confirmed your PR has the template embedded:

image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI approved Approve running CI for fork locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants