ios: Use main background color as the splash screen #1697
Conversation
rajveermalviya
added
the
maintainer review
test/widgets/store_test.dart
Outdated
| // Once blockingFuture completes… | ||
| completer.complete(); | ||
| await tester.pump(); | ||
| await tester.pump(); |
There was a problem hiding this comment.
I am not sure why the extra tester.pump() is needed now.
There was a problem hiding this comment.
Puzzling, yeah. I just spent a few minutes investigating; and I don't see why this would have made a difference, but it does.
This potentially indicates a real bug, though purely in performance and not correctness: it may mean that the loading screen sticks around for a frame longer than it needs to before things get going, so that the user experiences the app starting up a bit more slowly.
I'll add a TODO comment, here and the other instance below. This code looks wrong — why is this extra pump needed? — and code that looks wrong should have a comment so the reader doesn't have to wonder about it.
|
Thanks, this looks great! Can we add bidirectional comments on the new "colorset" and on |
rajveermalviya
force-pushed
the
pr-ios-splash
branch
from
3b094dd to
bfa684c
Compare
Unfortunately it doesn't, anyway added the one-way comment on |
| "alpha" : "1.000", | ||
| "blue" : "0.941", | ||
| "green" : "0.941", | ||
| "red" : "0.941" |
There was a problem hiding this comment.
nit: Ah I think we can use a hex representation, to make it easier to compare with DesignVariables.mainBackground?
Like in zulip-mobile:
"blue" : "0xFE",
"green" : "0x92",
"red" : "0x64"
There was a problem hiding this comment.
Hopefully that would be an NFC change; hopefully it wasn't a factor in my failing to get things to work (as I mentioned in discussion)
There was a problem hiding this comment.
Updated, and tested that the behaviour is same.
rajveermalviya
force-pushed
the
pr-ios-splash
branch
from
bfa684c to
2f1fb50
Compare
|
Thanks, LGTM! @gnprice PTAL. |
chrisbobbe
added
integration review
|
Also GitHub shows a rebase conflict in theme.dart |
rajveermalviya
force-pushed
the
pr-ios-splash
branch
from
2f1fb50 to
db28e43
Compare
|
(Rebased to main.) |
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks to both of you for taking care of this! Looks good, except that puzzle about the extra frame that the tests suggest is now needed. I'll add TODO comments and merge.
test/widgets/store_test.dart
Outdated
| // Once blockingFuture completes… | ||
| completer.complete(); | ||
| await tester.pump(); | ||
| await tester.pump(); |
There was a problem hiding this comment.
Puzzling, yeah. I just spent a few minutes investigating; and I don't see why this would have made a difference, but it does.
This potentially indicates a real bug, though purely in performance and not correctness: it may mean that the loading screen sticks around for a frame longer than it needs to before things get going, so that the user experiences the app starting up a bit more slowly.
I'll add a TODO comment, here and the other instance below. This code looks wrong — why is this extra pump needed? — and code that looks wrong should have a comment so the reader doesn't have to wonder about it.
This removes the un-styled CircularProgressIndicator that was displayed for a brief moment, instead the user now just sees the default background color. [greg: added TODO-comments on the extra frame that seems to now be needed]
Use `DesignVariables.mainBackground` as the iOS launch screen colors. Discussion: https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/splash.20screen.20on.20iOS/near/2215883 Fixes: zulip#1149
3 participants
Use
DesignVariables.mainBackgroundas the iOS launch screen colors.Discussion:
https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/splash.20screen.20on.20iOS/near/2215883
Fixes: #1149