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

Expand the configuration code example to give some more context #107

Merged
merged 8 commits into from
Oct 12, 2024

Conversation

cakeGit
Copy link
Contributor

@cakeGit cakeGit commented Jun 8, 2024

I thought the code examples on this page were a bit short, so I've just extended it to help people get started with configs easier


Preview URL: https://pr-107.neoforged-docs-previews.pages.dev

cakeGit added 2 commits June 8, 2024 18:45
Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 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 contribution! Provided some thoughts on some of the syntax and flow in the rest of the document.

docs/misc/config.md Outdated Show resolved Hide resolved
docs/misc/config.md Outdated Show resolved Hide resolved
docs/misc/config.md Outdated Show resolved Hide resolved
docs/misc/config.md Outdated Show resolved Hide resolved
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) June 12, 2024 12:55 Active
@neoforged-pages-deployments
Copy link

neoforged-pages-deployments bot commented Jun 12, 2024

Deploying with Cloudflare Pages

Name Result
Last commit: 422aca381327268045813ead02b655c8e9452eb7
Status: ✅ Deploy successful!
Preview URL: https://840bc641.neoforged-docs-previews.pages.dev
PR Preview URL: https://pr-107.neoforged-docs-previews.pages.dev

@IchHabeHunger54 IchHabeHunger54 added the addition Adding or rewriting information. label Jun 12, 2024
Copy link
Member

@IchHabeHunger54 IchHabeHunger54 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 overall to me, requesting one change on top of Champ's comments.

docs/misc/config.md Outdated Show resolved Hide resolved
Co-authored-by: IchHabeHunger54 <[email protected]>
IchHabeHunger54
IchHabeHunger54 previously approved these changes Aug 7, 2024
Copy link
Member

@IchHabeHunger54 IchHabeHunger54 left a comment

Choose a reason for hiding this comment

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

This article will probably be rewritten as a whole in the future, however for now this is definitely a good addition.

Copy link
Contributor

@ChampionAsh5357 ChampionAsh5357 left a comment

Choose a reason for hiding this comment

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

I saw you marked my comments as resolved, but it seems like you ignored them. Would you mind giving some justification as to why?

@cakeGit
Copy link
Contributor Author

cakeGit commented Oct 4, 2024

Unsure why they weren't resolved, I think there was a missing commit, check the new commit though it should be in there, :)

I saw you marked my comments as resolved, but it seems like you ignored them. Would you mind giving some justification as to why?

@ChampionAsh5357
Copy link
Contributor

Unsure why they weren't resolved, I think there was a missing commit, check the new commit though it should be in there, :)

I unresolved the two comments that weren't addressed. Could you please explain your decisions for these two (public constructor and non-public fields).

@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) October 4, 2024 14:37 Active
@cakeGit
Copy link
Contributor Author

cakeGit commented Oct 5, 2024

Resolved the remaining issues

@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) October 12, 2024 19:55 Active
@IchHabeHunger54 IchHabeHunger54 merged commit ee7abec into neoforged:main Oct 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition Adding or rewriting information.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants