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: Integration of flame_riverpod #2367

Merged
merged 49 commits into from
Dec 1, 2023
Merged

feat: Integration of flame_riverpod #2367

merged 49 commits into from
Dec 1, 2023

Conversation

markvideon
Copy link
Contributor

Description

Previously discussed with @spydon and referenced in issue #2353, flame_riverpod is to be integrated into the monorepo

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • 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 or docs.

Breaking Change?

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

Related Issues

Contribution toward #2353

@markvideon
Copy link
Contributor Author

The example Flutter project contains a widget test that accesses lifecycle methods (eg processQueue), which is currently annotated as protected. Perhaps it could be visibleForTesting?

The widget test in question is a standard Flutter widget test that compares the contents of a Flutter Text Widget against a Flame Text Component for equality. This is to validate that both widgets are reading from the StreamProvider defined in the project.

Copy link
Contributor

@st-pasha st-pasha left a comment

Choose a reason for hiding this comment

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

Could you please remove all the files that are not part of the library? Such as linux/, macos/, web/, and windows/ folders.

@spydon
Copy link
Member

spydon commented Feb 26, 2023

The example Flutter project contains a widget test that accesses lifecycle methods (eg processQueue), which is currently annotated as protected. Perhaps it could be visibleForTesting?

It can be both, so you can just add that meta tag there and it should be fine.

packages/flame_riverpod/LICENSE Outdated Show resolved Hide resolved
packages/flame_riverpod/README.md Outdated Show resolved Hide resolved
packages/flame_riverpod/example/.gitignore Outdated Show resolved Hide resolved
packages/flame_riverpod/example/README.md Outdated Show resolved Hide resolved
packages/flame_riverpod/example/lib/main.dart Outdated Show resolved Hide resolved
packages/flame_riverpod/example/test/widget_test.dart Outdated Show resolved Hide resolved
packages/flame_riverpod/pubspec.yaml Outdated Show resolved Hide resolved
packages/flame_riverpod/pubspec.yaml Outdated Show resolved Hide resolved
packages/flame_riverpod/pubspec.yaml Outdated Show resolved Hide resolved
markvideon and others added 15 commits February 27, 2023 14:18
Added package name to beginning of file as per suggestion

Co-authored-by: Lukas Klingsbo <[email protected]>
…nually in test. (Change yet to be committed)
…nge requests:

- Updated flame_riverpod website field
- Updated flame_riverpod description
- Bumped up flame version number to 1.6.0

Additionally, removed whitespace
…Test due to error in automated Github action
@markvideon markvideon requested a review from spydon February 27, 2023 05:06
@spydon spydon mentioned this pull request Feb 27, 2023
6 tasks
packages/flame_riverpod/README.md Outdated Show resolved Hide resolved
packages/flame_riverpod/README.md Outdated Show resolved Hide resolved
packages/flame_riverpod/README.md Outdated Show resolved Hide resolved
packages/flame_riverpod/lib/src/component_ref.dart Outdated Show resolved Hide resolved
packages/flame_riverpod/lib/src/component_ref.dart Outdated Show resolved Hide resolved
@spydon spydon marked this pull request as draft April 16, 2023 14:18
@garysm
Copy link

garysm commented Sep 18, 2023

@markvideon Can we expect an update for this?

@markvideon
Copy link
Contributor Author

@markvideon Can we expect an update for this?

Yes

@markvideon markvideon marked this pull request as ready for review November 30, 2023 05:02
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 great! 😍
Just a few minor comments, nothing on the implementation.

.github/.cspell/words_dictionary.txt Outdated Show resolved Hide resolved
packages/flame_riverpod/example/.gitignore Outdated Show resolved Hide resolved
packages/flame_riverpod/example/lib/main.dart Outdated Show resolved Hide resolved
packages/flame_riverpod/example/pubspec.yaml Outdated Show resolved Hide resolved
packages/flame_riverpod/lib/src/widget.dart Show resolved Hide resolved
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.

Lgtm! 🥳

@spydon spydon dismissed st-pasha’s stale review November 30, 2023 09:35

Not active anymore :(

@spydon
Copy link
Member

spydon commented Nov 30, 2023

@markvideon I missed one thing in my review, docs are missing.
Similar to these ones: https://github.com/flame-engine/flame/tree/main/doc/bridge_packages/flame_bloc

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.

Two minor comments, otherwise I think we're good to go! 😍

doc/bridge_packages/flame_riverpod/flame_riverpod.md Outdated Show resolved Hide resolved
doc/bridge_packages/flame_riverpod/riverpod.md Outdated Show resolved Hide resolved
markvideon and others added 3 commits December 1, 2023 18:57
fix: Spacing as per suggestion

Co-authored-by: Lukas Klingsbo <[email protected]>
docs: Change of link as per suggestion

Co-authored-by: Lukas Klingsbo <[email protected]>
@spydon spydon enabled auto-merge (squash) December 1, 2023 08:32
@spydon spydon merged commit 0c74560 into flame-engine:main Dec 1, 2023
8 checks passed
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.

6 participants