search

lichess-org / mobile

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

feat: add study screen #1111

Closed tom-anders closed 1 week ago

tom-anders commented 3 weeks ago

Feature set is the same as the old app for now, I'll add support for interactive studies in another PR.

Chat is also missing for now, I'll have to figure out how to use the websocket for that.

@veloce The build fails because this PR depends on some of the other study PRs, namely https://github.com/lichess-org/mobile/pull/1109 https://github.com/lichess-org/mobile/pull/1077 and https://github.com/lichess-org/mobile/pull/990. If you want to try out the whole feature, it's probably easiest to create a local branch based on this PR and then rebase it onto the other ones.

study_screen.webm

veloce commented 3 weeks ago

Love it! The UI is so simple yet intuitive, that's how it should be done imo.

tom-anders commented 3 weeks ago

(rebased against main)

Mansour-J commented 2 weeks ago

This looks great! Having the list of chapters come from a drawer could really improve it, freeing up more space for the chapter names.

The dialogue section feels a bit cramped at the moment— The drawer gives it a bit more room and could make the content look less tight.

Overall, it’s amazing work! 👏

tom-anders commented 2 weeks ago

This looks great! Having the list of chapters come from a drawer could really improve it, freeing up more space for the chapter names.

The dialogue section feels a bit cramped at the moment— The drawer gives it a bit more room and could make the content look less tight.

Overall, it’s amazing work! 👏

Maybe we could simply display a full-screen dialog to free up as much space as possible for the chapter titles?

Mansour-J commented 2 weeks ago

Overall, it’s amazing work! 👏

Maybe we could simply display a full-screen dialog to free up as much space as possible for the chapter titles?

@tom-anders Not sure how a full screen modal looks like.

is this how it looks? (https://www.youtube.com/watch?v=8BkXiQD-zkU)

veloce commented 2 weeks ago

In case you missed it @tom-anders , I think now you can rebase on main to avoid conflicts and make the build pass.

tom-anders commented 2 weeks ago

Thanks for the heads-up, rebased now :)

Also, study chapter selection is now a separate screen (and the titles wrap, so they'll never be cut off):

Screenshot_1730833542

veloce commented 2 weeks ago

Isn't it a bit weird to have the chapters on a different screen? Pushing a new screen to the stack and tap an item there to make a change on the screen below seems not very intuitive. If we do that it should at least be a full screen modal (fullScreenDialog param of Navigator.push) that closes itself automatically when a chapter is selected.

It could also be a bottom sheet (like we have for settings).

tom-anders commented 2 weeks ago

Isn't it a bit weird to have the chapters on a different screen? Pushing a new screen to the stack and tap an item there to make a change on the screen below seems not very intuitive. If we do that it should at least be a full screen modal (fullScreenDialog param of Navigator.push) that closes itself automatically when a chapter is selected.

It could also be a bottom sheet (like we have for settings).

Will try out a bottom sheet later today 👍

tom-anders commented 2 weeks ago

Thanks for the review!

Moved chapters to a bottom sheet now:

chapters.webm

veloce commented 2 weeks ago

Thanks for the update @tom-anders !

Just quickly tested study feature and it seems collapsing variation is not working properly anymore:

https://github.com/user-attachments/assets/a8231c87-b8f8-48c7-9cb8-66bfea86c196

tom-anders commented 2 weeks ago

Thanks for the update @tom-anders !

Just quickly tested study feature and it seems collapsing variation is not working properly anymore: collapse_bug.mov

Oh, good catch, sorry about that!

That's an interesting chain reaction actually :D I wanted to port https://github.com/lichess-org/mobile/pull/1137/commits/a4463522a45b6dd44fda4ca4a088bc4199e7792e to this PR, but in that commit I called the new variable childrenToHide (instead of childrenToShow. So when merging the changes into StudyController in this branch, I added them to collapseVariations() instead of expandVariations(), messing up both collapsing and expanding lol.

veloce commented 2 weeks ago

Thanks for the fix!

Now that I'm playing with the collapsing/expanding, I find it a little disturbing (I know I merge the PR that submitted the choice, in video it looked fine but I hadn't actually tested it).

The "plus" and "minus" buttons are disappearing once they are tapped, which is not the best UX experience. In order to work, only one button is needed and it should stay at the same place, changing its appearance with the collapsed state.

We can compare with a file explorer to have a good idea of what we should achieve:

https://github.com/user-attachments/assets/1ce06e1a-37be-4ac8-909f-534ae1bc4e46

It would be great to have animation when expanding/collapsing. But the most important first is to get the buttons right. I wonder if I should revert the PR that introduces the minus button, or if we can solve this quickly? Would it work if the "+" button was at the end of the line (same as minus) instead of being in the indented line? And we could show the first node with a "..." in the indented line in place of the +.

tom-anders commented 2 weeks ago

Hmm, might be not be a small fix, we'd have to change some logic of the TreeView widget again for that (including the code that draws the indent guides). I'd say let's revert the change that introduced the minus and I'll have another look at it in a new PR

tom-anders commented 2 weeks ago

Maybe we could even go full file Explorer mode? Have a v icon in front of the line when it's expanded and a > icon if the line is collapsed?

Anyways, I'll revert the commit in this PR and keep you updated about this idea

tom-anders commented 1 week ago

(rebased to fix conflicts)

tom-anders commented 1 week ago

Also, cherry-picked the bug fix https://github.com/lichess-org/mobile/pull/1111/commits/f291adbdd0902ebfd465869b0adfe30e3a737cae from https://github.com/lichess-org/mobile/pull/1137 (which was reverted, but the bugfix was unrelated)

veloce commented 1 week ago

Also, cherry-picked the bug fix f291adb from #1137 (which was reverted, but the bugfix was unrelated)

Thanks! It should be cherry-picked on main too, probably. But no big deal, we'll merge this eventually.

tom-anders commented 1 week ago

Also, cherry-picked the bug fix f291adb from #1137 (which was reverted, but the bugfix was unrelated)

Thanks! It should be cherry-picked on main too, probably. But no big deal, we'll merge this eventually.

Yeah depends on when you're planning this PR - feel free to cherry-pick to main as well

tom-anders commented 1 week ago

Thanks for the review again :) I rebased https://github.com/lichess-org/mobile/pull/1128 so it's ready for review now as well