lichess-org / mobile

Lichess mobile app v2
GNU General Public License v3.0
1.35k stars 196 forks source link

Add accept/decline buttons to game result dialog after rematch offer received #1153

Closed Jimima closed 1 day ago

Jimima commented 1 week ago

Just looking at this, seems straightforward to do but just wondering how to handle the UI. Initial implementation (not styled). UI suggestions welcome!

Relates to https://github.com/lichess-org/mobile/issues/1148

veloce commented 1 week ago

In some language "Rematch offered" can be a long text, so I'm not sure it would display nicely on one line.

We could instead have the "Rematch offered" on one line and below the centered buttons "Accept Decline".

Jimima commented 1 week ago

One thing I'm noticing here is that swapping out the button to offer a rematch might be problematic in the case that the user is about the offer a rematch and the UX changes under them. Not sure how big of a problem this is in reality

Jimima commented 1 week ago

Alternate approach

veloce commented 1 week ago

Good question, now sure either how to handle this properly. Perhaps it is not that much a problem.

Also, how about having the whole rematch offer block stand out with a light background color for instance?

Jimima commented 1 week ago

@veloce thanks, I am a bit worried if we add a label above the buttons it will cause a layout shift which could be annoying. I wonder how clear the latest approach is, the "Accept rematch" button implies one has been offered but it's maybe not clear. I think the design is clean, at least

veloce commented 1 week ago

@veloce thanks, I am a bit worried if we add a label above the buttons it will cause a layout shift which could be annoying. I wonder how clear the latest approach is, the "Accept rematch" button implies one has been offered but it's maybe not clear. I think the design is clean, at least

Layout shift can be handled by animating the change I think. Like a height change animation coupled with an opacity animation maybe?

veloce commented 1 week ago

Feel free to experiment with this and share screen recording of the UX that you find works well also.

Jimima commented 1 week ago

Sure I have not really worked with animations so I will have a play around, appreciate the feedback

veloce commented 1 week ago

Animation in flutter is not that hard, once you've chosen the right approach:

😆

Jimima commented 1 week ago

Here is an attempt at using animation, I think it works. In summary:

  1. Remove rematch buttons when opponent offers rematch
  2. Add (hidden) label to indicate the opponent has offered a rematch
  3. Add (hidden) buttons to accent or decline
  4. Animate dialog to fit new content (over 400ms)
  5. Show label buttons once animation is complete

400ms based on gut feel and this

https://github.com/user-attachments/assets/4be04bf5-fe16-4b50-b9b7-278b8d938f73

Boubou78000 commented 1 week ago

I think it would be better without animation 😅 Or with a faster one

Jimima commented 1 week ago

It can be faster I thought it would be easier to show it a bit slower for review. Not much point going too fast without simply removing it and I think without it the layout shift could be annoying. Happy for someone to make the call, ultimately, just trying things out at the moment

Boubou78000 commented 1 week ago

Alternate approach

This one is good 👍

veloce commented 1 week ago

As suggested by @Boubou78000 your alternate approach is better to avoid the layout shift (not showing the "Your opponent offered a rematch").

We'd still use a FadeTransition (with a duration ~300ms I'd say) when the widgets change.

It should be pretty obvious that the opponent offers a rematch, seeing the labels "Accept rematch", "Decline".

Can you post a video showing all the states:

Thanks!

Jimima commented 1 week ago

Yeah I'm a little concerned that the buttons alone are not enough but happy to go with that for now. I suspect there will be feedback if it's not clear! Will make the changes, no problem

Jimima commented 1 week ago

Changes as discussed. Video:

https://github.com/user-attachments/assets/f42c7600-50cd-4b6a-889e-3662be39513d

Boubou78000 commented 1 week ago

@Jimima there is still the rematch button when clicked it? Should it not just show a cancel rematch button, not both? Weird 😅

Jimima commented 1 week ago

Oops, yeah that's just a bug. I should have noticed that. Should be better now

https://github.com/user-attachments/assets/36a3dd1d-abb9-4671-bac1-a5a6d444c065

Boubou78000 commented 1 week ago

Oops, yeah that's just a bug. I should have noticed that. Should be better now

https://github.com/user-attachments/assets/36a3dd1d-abb9-4671-bac1-a5a6d444c065

Nice 👍

I just noticed another small thing: the rematch and the cancel rematch buttons are the same design, so MAYBE add a box or anything, just to see a visual change 🤔

Othetwise I think this PR is ready to merge 😃

veloce commented 2 days ago

Yeah I'm a little concerned that the buttons alone are not enough but happy to go with that for now. I suspect there will be feedback if it's not clear! Will make the changes, no problem

Now that I see it live, I'm not sure anymore we should do it like that. Sorry about changing my mind, but this is a sensitive feature because related to games and used a lot.

Actually the animation wasn't a bad idea. It shows a change that the use does not expect, so animating it makes sense because we have the time to see something is happening. I hope you kept the commit? If you can revert to the state where we had the animation that'd be great. But with one change: instead of removing the "Offer rematch" button when opponent already offers it, I'd keep the button in a disabled state. @Jimima Thank you very much!

Jimima commented 2 days ago

Sure no problem, can revert I think and keeping the button should be a simple enough change.

Jimima commented 2 days ago

@veloce I thought I had the commit but apparently not, IDE local history had it though :-)

I have pushed up the animated version.

I had a quick look at how I might keep the rematch button but disable it and I couldn't work out how the enabled/disabled state was being managed. Can you enlighten me?

Jimima commented 2 days ago

Actually never mind, I figured it out I think. Here is a version as requested (I think)

https://github.com/user-attachments/assets/a17e08de-8479-43ee-a595-d5db735b1475

veloce commented 2 days ago

Yeah I think it's better like that! Thank you. Looks like the lint is failing.

Jimima commented 2 days ago

It is. I have a bit of tidying up to do anyway so I will do that and let you know

tom-anders commented 2 days ago

@veloce I thought I had the commit but apparently not, IDE local history had it though :-)

I have pushed up the animated version.

I had a quick look at how I might keep the rematch button but disable it and I couldn't work out how the enabled/disabled state was being managed. Can you enlighten me?

FYI for next time, in case your IDE does not have the code in its undo history: you can recover "lost" commits (they're not actually lost, just unreachable) via git reflog

Boubou78000 commented 2 days ago

Actually never mind, I figured it out I think. Here is a version as requested (I think)

https://github.com/user-attachments/assets/a17e08de-8479-43ee-a595-d5db735b1475

I think there is a small padding problem between the rematch button and the accept rematch one... But I'm not sure...

Jimima commented 2 days ago

Latest (possibly final 🙂) iteration

Edit: I should probably do a bit more testing of edge cases but here it is in the meantime

https://github.com/user-attachments/assets/571f38c7-f6da-4260-9eee-881802f84f1c

Boubou78000 commented 2 days ago

Latest (possibly final 🙂) iteration

Edit: I should probably do a bit more testing of edge cases but here it is in the meantime

https://github.com/user-attachments/assets/571f38c7-f6da-4260-9eee-881802f84f1c

Again, it looks like there is extra space above the rematch button...

Jimima commented 2 days ago

You're right I'll take a look at that tomorrow

Jimima commented 2 days ago
Screenshot 2024-11-19 at 22 25 31
Jimima commented 1 day ago

@veloce I think I addressed your comments. The AnimatedCrossfade works well. I used the curves to somewhat mimic the animation I had before but it's a little nicer, much nicer in code. You can play around with the curves to your hearts content. I think what I have is good but you (or anyone) might prefer a slightly different UX when fading in/out

https://github.com/user-attachments/assets/72fa331c-ae3e-47ff-8ef9-d8a11a90aa12

Boubou78000 commented 1 day ago

Again, there is extra space above the rematch button, maybe because of the box around accept rematch?

I mean that there is not the same distance between the accept rematch button (text) and the rematch button and between the rematch button the new opponent button...

Jimima commented 1 day ago

I added some padding because otherwise the layout felt cramped. This looks well spaced to my eyes

Boubou78000 commented 1 day ago

Ok. Maybe keep padding, but less?