Confetti after order confirmed #823
Conversation
|
It would be good to document the minimum supported flutter version in readme, and someday perhaps use something like: https://fvm.app/ to manage flutter versions |
|
@Restioson any chance for a short recording how it looks like? :) |
|
Hi Mariusz, I'm away until the 4th and I can try do this when I get back :)
…On Mon, 26 Jun 2023, 02:16 Mariusz Klochowicz, ***@***.***> wrote:
@Restioson <https://github.com/Restioson> any chance for a short
recording how it looks like? :)
—
Reply to this email directly, view it on GitHub
<#823 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTBBNEOYEXKCCLJPVASLK3XNDIGHANCNFSM6AAAAAAZQLA4U4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Just gave this a try locally - looks great! Here's a gif that shows it for closing the order (any order filled will trigger it):
Should I fix the lints and we get it merged? |
There was a problem hiding this comment.
Just one question comment - not sure important; from my side this PR is ready to merge if we want to.
Note: I had some local changes that are not part of the PR:
modified: mobile/macos/Podfile.lock
modified: mobile/pubspec.lock
Note sure this is relevant though; might just be due to me having some outdated versions.
| } | ||
|
|
||
| class _OrderSubmissionStatusDialog extends State<OrderSubmissionStatusDialog> { | ||
| late final ConfettiController _confettiController; |
There was a problem hiding this comment.
❓Why are we defining the ConfettiController in the OrderSubmissionStatusDialog and the TradeScreen?
My understanding is that the dialog cannot be closed anymore before we reach the orderFilled state; so I think it should be enough to define the confetti on the OrderSubmissionStatusDialog.
I don't think it's wrong to define it on the underlying Tradescreen as well, but I'm not sure it's necessary.
|
It's probably a hangover from when I was still testing. Unfortunately I did
this last thing before I left on holiday (I am still away with no PC or
laptop) so I didn't have time to clear it up 😅
…On Thu, 29 Jun 2023, 05:53 Daniel Karzel, ***@***.***> wrote:
***@***.**** approved this pull request.
Just one question comment - not sure important; from my side this PR is
ready to merge if we want to.
Note: I had some local changes that are not part of the PR:
modified: mobile/macos/Podfile.lock
modified: mobile/pubspec.lock
Note sure this is relevant though; might just be due to me having some
outdated versions.
------------------------------
In mobile/lib/features/trade/order_submission_status_dialog.dart
<#823 (comment)>:
> @@ -26,24 +27,47 @@ class OrderSubmissionStatusDialog extends StatelessWidget {
this.insetPadding = const EdgeInsets.all(50),
this.navigateToRoute = ""});
+ @OverRide
+ State<OrderSubmissionStatusDialog> createState() => _OrderSubmissionStatusDialog();
+}
+
+class _OrderSubmissionStatusDialog extends State<OrderSubmissionStatusDialog> {
+ late final ConfettiController _confettiController;
❓Why are we defining the ConfettiController in the
OrderSubmissionStatusDialog *and* the TradeScreen?
My understanding is that the dialog cannot be closed anymore; so I think
it should be enough to define the confetti on the
OrderSubmissionStatusDialog.
I don't think it's wrong to define it on the underlying Tradescreen as
well, but I'm not sure it's necessary.
—
Reply to this email directly, view it on GitHub
<#823 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTBBNGMZWPBVZ4WPM6E653XNT33RANCNFSM6AAAAAAZQLA4U4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'd be grateful if someone can remove the unused controller first!
…On Fri, 30 Jun 2023, 07:06 Mariusz Klochowicz, ***@***.***> wrote:
***@***.**** approved this pull request.
looks awesome, let's get this in!
—
Reply to this email directly, view it on GitHub
<#823 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTBBNGSWCG7XVSLQ7KHALDXNZNFHANCNFSM6AAAAAAZQLA4U4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
awesome, I'll take care of this :) |
|
bors r+ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
This is on top of #820 and should wait for that to be merged first. This is intended to add a little hype to the new position and encourage the user to celebrate by sharing on Twitter :)
Right now it is just a little puff of confetti from the tick.
This needs flutter >3.7.2 to work, otherwise it will crash.
Fixes #822.