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

ChildBuilder docs improvement #8371

Merged

Conversation

JohnTheCoolingFan
Copy link
Contributor

Objective

Fixes #7230

Solution

Documented some side-effects

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Is this PR still a WIP? Make sure to put it in draft mode if you still have work to do.

I don't think this solves #7230. It only superficially changes two docs. Have you looked at the other methods and judged they were good as-is? If so, it is perfectly fine, but mention it please.

Otherwise, I don't think the provided modifications are improvements, I gave inline suggestions on possible improvements.

crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
@JohnTheCoolingFan JohnTheCoolingFan marked this pull request as draft April 13, 2023 12:42
@nicopap nicopap added C-Docs An addition or correction to our documentation A-Hierarchy Parent-child entity hierarchies labels Apr 13, 2023
@JohnTheCoolingFan JohnTheCoolingFan marked this pull request as ready for review April 13, 2023 17:40
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I think this PR is value added, but need some improvements still.

crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Some minor changes and I think it's good to go.

crates/bevy_hierarchy/src/child_builder.rs Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
crates/bevy_hierarchy/src/child_builder.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I'm still wondering if the wording about "this entity's parent" in ChildBuilder is not misleading. But it's already a net improvement, so it's good for me.

@james7132 james7132 self-requested a review April 16, 2023 04:58
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

A substantial improvement. Approving and merging.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 22, 2023
Merged via the queue into bevyengine:main with commit 15a96ad Apr 22, 2023
Estus-Dev pushed a commit to Estus-Dev/bevy that referenced this pull request Jul 10, 2023
# Objective

Fixes bevyengine#7230 

## Solution

Documented some side-effects

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies C-Docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve ChildBuilder documentation to ensure a consistent style
3 participants