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: Added RouterComponent #1755

Merged
merged 66 commits into from
Aug 10, 2022
Merged

feat: Added RouterComponent #1755

merged 66 commits into from
Aug 10, 2022

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented Jun 26, 2022

Description

This PR adds the RouterComponent (see the docs for the description of its functionality).

Screen.Recording.2022-06-26.at.1.35.32.PM.mov

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

Prerequisite: #1781
Closes #1375

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 very good, looking forward to using it!
I think the example can be simplified quite a bit by using already existing Flame components though, to keep it more on point.

doc/flame/examples/lib/navigator.dart Outdated Show resolved Hide resolved
doc/flame/examples/lib/navigator.dart Outdated Show resolved Hide resolved
doc/flame/examples/lib/navigator.dart Outdated Show resolved Hide resolved
doc/flame/examples/lib/navigator.dart Outdated Show resolved Hide resolved
doc/flame/examples/lib/navigator.dart Outdated Show resolved Hide resolved
doc/flame/navigator.md Outdated Show resolved Hide resolved
packages/flame/lib/src/components/navigator.dart Outdated Show resolved Hide resolved
packages/flame/lib/src/components/navigator.dart Outdated Show resolved Hide resolved
packages/flame/lib/src/page_render_effect.dart Outdated Show resolved Hide resolved
@st-pasha st-pasha marked this pull request as ready for review July 4, 2022 08:21
@spydon
Copy link
Member

spydon commented Jul 7, 2022

Maybe there is a better term than "Route", but I don't think it's the "Scene".

I think that Route is fine, and I think that we can probably introduce a scene concept later (which basically is just an empty component at the moment).

@alestiago
Copy link
Contributor

alestiago commented Jul 16, 2022

I think this would benefit from #1799 . Where the Navigator would be an InheritedComponent. Similar to how Flutter deals with navigation.

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.

I think that it looks good!
I'm thinking whether it is worth having generics on the NavigatorComponent so that it can be used in a more safe way than just plain strings. The cons that I see of that though is:

  • It might be harder for the user to get into using it at first
  • That generic would also spread to the Route.

packages/flame/lib/src/components/navigator_component.dart Outdated Show resolved Hide resolved
spydon pushed a commit that referenced this pull request Jul 18, 2022
The new class allows applying visual filters to canvas drawing operations. It is conceptually similar to the LayerProcessor class, except that it applies directly to a canvas instead of a Picture.

This functionality was extracted from PR #1755.

NB: the guitar image was taken from here, which is a public domain image, i.e. licensed under CC0 1.0.
@st-pasha
Copy link
Contributor Author

I think this PR has no action items left, @spydon WDYT?

@spydon
Copy link
Member

spydon commented Jul 28, 2022

I think this PR has no action items left, @spydon WDYT?

If I remember correctly, there was a discussion about adding generics to the routes so that enums for example could be used, and then have a default implementation that just handled strings?

@st-pasha
Copy link
Contributor Author

If I remember correctly, there was a discussion about adding generics to the routes so that enums for example could be used, and then have a default implementation that just handled strings?

My understanding was that because enums/strings have no common super-type, then using generics here would be counter-productive as it will lead for less type-safety (dynamic as the inferred type).

An additional problem with generic types is that it wouldn't be able to accommodate some of the existing features like generated route names.

@spydon
Copy link
Member

spydon commented Jul 29, 2022

My understanding was that because enums/strings have no common super-type, then using generics here would be counter-productive as it will lead for less type-safety (dynamic as the inferred type).

An additional problem with generic types is that it wouldn't be able to accommodate some of the existing features like generated route names.

Right, that was it, now I remember.

@spydon spydon requested a review from a team July 29, 2022 14:39
@spydon spydon requested a review from a team July 29, 2022 14:40
@spydon
Copy link
Member

spydon commented Jul 29, 2022

@st-pasha is there a plan for how to support routes opening overlays? I feel that is pretty important so that users aren't locked in to create menus etc as flame components.

@st-pasha
Copy link
Contributor Author

Yeah, we have issue #1762 for that.

Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

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

I am still not sure about the Navigator name, internally me and @spydon discussed a little bit and the RouterComponent name could also be a good fit and I think I like it more.

Either way, gonna leave my approval in here.

@spydon
Copy link
Member

spydon commented Aug 8, 2022

I am still not sure about the Navigator name, internally me and @spydon discussed a little bit and the RouterComponent name could also be a good fit and I think I like it more.

Either way, gonna leave my approval in here.

For me both RouterComponent and NavigatorComponent are fine, but if you prefer RouterComponent maybe we should go with that then so that we can get this merged. :)

@st-pasha st-pasha changed the title feat: Added NavigatorComponent feat: Added RouterComponent Aug 10, 2022
@spydon spydon merged commit 24092bd into flame-engine:main Aug 10, 2022
@st-pasha st-pasha deleted the ps.navigator branch August 10, 2022 16:30
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.

Create Navigator component
6 participants