-
-
Notifications
You must be signed in to change notification settings - Fork 905
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
Conversation
There was a problem hiding this 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.
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). |
I think this would benefit from #1799 . Where the |
There was a problem hiding this 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
.
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.
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? |
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 ( 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. |
@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. |
Yeah, we have issue #1762 for that. |
There was a problem hiding this 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.
For me both |
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
fix:
,feat:
,docs:
etc).docs
and added dartdoc comments with///
.examples
.Breaking Change
Related Issues
Prerequisite: #1781Closes #1375