Add hero widget for profile picture animation #1131
Conversation
|
Hi! Thanks for contributing, please fix the linting errors first 😅, thanks! |
| @override | ||
| Widget build(BuildContext context) { | ||
| if (onLoad == null) { |
There was a problem hiding this comment.
It think it is cleaner to simply surround the callback with the condition, this is repeated below.
There was a problem hiding this comment.
If the callback is surrounded by if (onLoad != null) {...} the variable _loading should be also set to false so the condition else { _loading = false; } is needed. Do you still prefer this approach over the implemented one?
There was a problem hiding this comment.
Hm, I meant surround the add callback statement, not the callback body
There was a problem hiding this comment.
But still, I get that, can we set loading and onLoad not null?
There was a problem hiding this comment.
Yep, it is feasible, but I think that in this case, the return early pattern makes the code simpler and easier to read
There was a problem hiding this comment.
Yeah, seems like a tradeoff. In that case, I'll leave it to your preference.
| ), | ||
| if (decorationImage.hasData) | ||
| { | ||
| Navigator.push( |
There was a problem hiding this comment.
Is there any way to keep the named route and pass a parameter, to keep consistency?
There was a problem hiding this comment.
Maybe. I think pushNamed can receive arguments and then be used on RouteSettings.
I'll try 🚀
| @@ -23,6 +23,14 @@ class CourseUnitDetailPageView extends StatefulWidget { | |||
|
|
|||
| class CourseUnitDetailPageViewState | |||
| extends SecondaryPageViewState<CourseUnitDetailPageView> { | |||
| @override | |||
| void initState() { | |||
There was a problem hiding this comment.
Can't you achieve this simply calling the construction? This field should be private, I think.
niaefeup-admin
force-pushed
the
develop
branch
from
417c250 to
bd61b50
Compare
|
I love to see that in the app! What can we do to make it possible? |
From what I remember, the hero widget was working correctly. However, I suppose that the usage of this particular widget triggered some internal application behaviour which conflicts with the test... |
Closes #1120
Add hero widget for profile picture animation
Review checklist
whatsnew/whatsnew-pt-PTchangelog.mdwith the change