Skip to content

Fixes #18740, added the missing documentation for B parameter on Trigger. #18914

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mhsalem36
Copy link
Contributor

Objective

Solution

  • Rewrote the documentation of Trigger and described how the B parameter in Trigger is used.

Testing

  • No testing needed since it is documentation.

@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Apr 24, 2025
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 24, 2025
Comment on lines +31 to +33
/// See [Trigger::propagate] for more information. It also provides a [Bundle] type representing a set of
/// components associated with the triggered event. This would be useful to access the components or data
/// that are related to the event.
Copy link
Member

Choose a reason for hiding this comment

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

Part of the confusion I was having with the B parameter is that it doesn't seem to filter its contained components by default. Instead, I believe there is some extra step needed when defining the observer such that it can actually be used. I think pointing users to that method(s) and explaining how to use it could be a bit more useful.

Comment on lines +31 to +33
/// See [Trigger::propagate] for more information. It also provides a [Bundle] type representing a set of
/// components associated with the triggered event. This would be useful to access the components or data
/// that are related to the event.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably also mention how this bundle parameter will filter out entities that don't contain all components in the bundle (it could be the other way around, I forget haha)

Comment on lines +29 to +30
/// A type that encapsulates information about a triggered [Event] during a specific [Observer] run. It includes
/// the [Event] data, the corresponding [Entity] that triggered it, and details about event propagation.
Copy link
Member

Choose a reason for hiding this comment

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

Two minor nitpicks:

  • The links should probably continue to use the backticks to indicate to the user that these are in fact traits/types
  • Normally we recommend an empty line after the first sentence/summary

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 5, 2025
@alice-i-cecile alice-i-cecile moved this to Observer overhaul in Alice's Work Planning May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
Status: Observer overhaul
Development

Successfully merging this pull request may close these issues.

Missing documentation for B parameter on Trigger
3 participants