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

feat: add HasAncestor mixin #1711

Merged
merged 12 commits into from
Jun 12, 2022
Merged

feat: add HasAncestor mixin #1711

merged 12 commits into from
Jun 12, 2022

Conversation

wolfenrain
Copy link
Contributor

Description

Add a mixin that ensures a component requires a certain ancestor in it's component tree.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

@wolfenrain wolfenrain self-assigned this Jun 7, 2022
@wolfenrain wolfenrain requested a review from a team June 7, 2022 13:51
}
assert(
_ancestor != null,
'An ancestor must be of type $T in the component tree',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'An ancestor must be of type $T in the component tree',
'Component of type $runtimeType has to have an ancestor of type $T in the component tree',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assertion will already throw with a stack trace that relates to the component. Not sure if this would have any benefit directly then?

If we do this we should do that for the ParentIsA and HasGameRef.

Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion on a better error message. Rather than searching the trace for the root of the problem, having the runtime type on the message can be helpful. If the case is for consistency, we can improve this and all related messages in a future PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea would love to do that in a follow up PR then! As we might have more assertions that could use some love probably

Copy link
Member

@spydon spydon 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, just some small nits

doc/flame/components.md Outdated Show resolved Hide resolved
doc/flame/components.md Show resolved Hide resolved
packages/flame/lib/src/components/mixins/has_ancestor.dart Outdated Show resolved Hide resolved
@wolfenrain wolfenrain enabled auto-merge (squash) June 12, 2022 11:47
@wolfenrain wolfenrain merged commit 987a44f into main Jun 12, 2022
@wolfenrain wolfenrain deleted the feat/add-has-ancestor-mixin branch June 12, 2022 11:54
st-pasha pushed a commit to st-pasha/flame that referenced this pull request Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants