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 Enchantments page #165

Merged
merged 43 commits into from
Oct 13, 2024
Merged

Add Enchantments page #165

merged 43 commits into from
Oct 13, 2024

Conversation

hjake123
Copy link
Contributor

@hjake123 hjake123 commented Sep 17, 2024

Adds a page under Resources > Server that describes how to make custom Enchantments and Enchantment Component Types in 1.21.x, as well as a small amount about how to work with the latter.

The relevant minecraft.wiki pages are frequently referenced and linked to, but the written content is my own.

Looking for feedback on this draft!

This should address issue #132


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

@hjake123 hjake123 changed the title Add a new Enchantments page Add Enchantments page Sep 17, 2024
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.

Thank you for the contribution!

Generally, this needs a lot more detail. It is too sparse to have an understanding on how to properly use the system. One example of this is the value effects, as that is hard to wrap your head around if you do not have a solid understanding of what is going on.

You should incorporate more information and examples of how one would implement these custom methods. Create a fake object that holds some arbitrary conditions. It should help the reader better understand how to create an enchantment.

docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
@hjake123 hjake123 marked this pull request as ready for review September 23, 2024 22:05
@hjake123
Copy link
Contributor Author

I think I've applied all these changes, let me know if there's more still to do!

@IchHabeHunger54 IchHabeHunger54 added the addition Adding or rewriting information. label Sep 27, 2024
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.

Good work! I will say there needs to be a lot more detail and narrative dialogue to help with the understanding of the document. It was hard to get a sense of how the system worked without already being aware of the logic itself. It's best if you take a step back and imagine if someone who had no knowledge of the system, would they be able to understand what you've written.

docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments.md Outdated Show resolved Hide resolved
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) October 3, 2024 00:41 Active
@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) October 7, 2024 15:38 Active
IchHabeHunger54
IchHabeHunger54 previously approved these changes Oct 8, 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.

Didn't go through it in detail again, but looks good overall. Approved by me.

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.

Great progress! However, I do think there needs to be a bit more explanation with your current reorganization. There are also some minor inaccuracies here and there that need to be fixed.

docs/resources/server/enchantments/index.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/index.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/index.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/index.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/index.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/builtin.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/builtin.md Show resolved Hide resolved
docs/resources/server/enchantments/builtin.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/builtin.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/builtin.md Outdated Show resolved Hide resolved
Co-authored-by: ChampionAsh5357 <[email protected]>
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.

Thank you for all the work you've done! Just two more changes: one syntax based and the other due to providing a understandable baseline.

Once these two are addressed, I don't believe I have any more comments for its current state.

docs/resources/server/enchantments/index.md Outdated Show resolved Hide resolved
docs/resources/server/enchantments/index.md Outdated Show resolved Hide resolved
@hjake123
Copy link
Contributor Author

Got it! Thank you so much for your patience and advice in leading me through this project.

@neoforged-pages-deployments neoforged-pages-deployments bot deployed to neoforged-docs-previews (Preview) October 12, 2024 20:47 Active
@ChampionAsh5357 ChampionAsh5357 merged commit 94bccc3 into neoforged:main Oct 13, 2024
1 check passed
@hjake123 hjake123 deleted the enchantments branch October 13, 2024 14:28
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.

3 participants