-
-
Notifications
You must be signed in to change notification settings - Fork 893
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: Loading builder for Route #3113
feat: Loading builder for Route #3113
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.
Lgtm, very clean implementation.
Could you add a simple test that just tests that both the loadingPage and then the page is shown?
It would be in here:
https://github.com/flame-engine/flame/blob/main/packages/flame/test/components/route_test.dart
wait ya , dont merge yet, adding test for this |
Don't merge yet. hi @spydon , sorry I was away for awhile. Just pushed new one with test. Anyways, I just have problem it making a full test to check if the loading component was mounted before it was removed. I was only able to check for loading component was removed and the builder component ( page component) was mounted right after it was removed. If you look at the file route_test.dart , I added test and also did add few comments , to explain the situation for the test coverage. Sorry I am not that good at coming with test, but I did manually check it by linking my version of flame repo to one of my old flame game, I mean my flame in pubspec refer to my git repo instead of pub.dev and test this feature I added, according to my observation it did work to what I expected. |
I mean does checking the loading component was removed technically means it also verifies that it was mounted before right . So if that is the case then the test I added already suffice ? |
// expect( | ||
// loadingComponent.isMounted, | ||
// isTrue, | ||
// ); | ||
// expect( | ||
// pageComponent.isMounted, | ||
// isFalse, | ||
// ); |
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 don't think I follow why this doesn't work, what are you getting here currently?
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'm sorry , False is being expected for 503, as for 507 it does pass
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.
Why is false expected for 503? Since the route is added it should be showing the loadingComponent
right?
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 just found, I made an error in the checking at line 514 is not accurate in this commit, it should be expecting True, but I checked there for False and it passed
That means my loading component was never mounted even before and after the main component was mounted?
Maybe, that mean my implementation may have some error ?
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'm guessing that due to how Future.delayed
works in tests the loadingComponent
was added and removed in the same tick. You could pass in a future to the _HeavyComponent
that you manually complete after you have checked that the loadingComponent
is mounted.
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.
Did you have any more time to look at this @PistonShot1?
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.
Sorry for the delays got caught up with another project, but I am trying to implement the approach you mentioned , to pass a future that I can manually complete, I am not exactly sure how to implement this . I am assuming with Completer
or sort ?
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.
Yeah, Completer
should work
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.
the test for checking if the loadingComponent
was mounted and remove, both doesn't pass (at line 499 and at line 509), as I have commented, it is some how implying that the loadingPage was never mounted. I am not sure why, but the changes I added at packages/flame/lib/src/components/route.dart
(line 146-151), supposed have mounted and removed after awaiting _page!.loaded;
…ot1/flame into loadingBuilder-addition
@PistonShot1 have you made any progress on this? :) |
@spydon . I have fixed it, sorry took so long for such a small feature addition, I gave up after the test keep on fail, but as you suspected it was the way I wrote the test, now it works fine and should be working and passed the expected tests |
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.
Lgtm, thanks for your contribution!
Description
This change is mainly to make it easier for specifying loading screen along when routing between component using the
RouterComponent
. Currently the change is only made onRoute
, that now have optionalloadingBuilder
to Route constructor which would expect aComponent
, which would be added as the loading page/screen whileonLoad
of thebuilder
(the main routing component) to be completed.Checklist
docs
and added dartdoc comments with///
.examples
ordocs
.Breaking Change?