Skip to content

Conversation

@tom-anders
Copy link
Collaborator

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 #1109 #1077 and #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
Copy link
Contributor

veloce commented Oct 27, 2024

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

@tom-anders
Copy link
Collaborator Author

(rebased against main)

@Mansour-J
Copy link

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
Copy link
Collaborator Author

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
Copy link

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
Copy link
Contributor

veloce commented Nov 5, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

veloce commented Nov 6, 2024

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
Copy link
Collaborator Author

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 👍

Copy link
Contributor

@veloce veloce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! Excited to get this in the app :)

@tom-anders
Copy link
Collaborator Author

Thanks for the review!

Moved chapters to a bottom sheet now:

chapters.webm

@veloce
Copy link
Contributor

veloce commented Nov 7, 2024

Thanks for the update @tom-anders !

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

collapse_bug.mov

@tom-anders
Copy link
Collaborator Author

tom-anders commented Nov 7, 2024

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 a446352 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
Copy link
Contributor

veloce commented Nov 7, 2024

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:

Enregistrement.de.l.ecran.2024-11-07.a.17.56.12.mov

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
Copy link
Collaborator Author

tom-anders commented Nov 7, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

(rebased to fix conflicts)

@tom-anders
Copy link
Collaborator Author

tom-anders commented Nov 10, 2024

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

@veloce
Copy link
Contributor

veloce commented Nov 12, 2024

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
Copy link
Collaborator Author

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

@veloce veloce merged commit 00ef719 into lichess-org:main Nov 12, 2024
1 check passed
@tom-anders
Copy link
Collaborator Author

Thanks for the review again :) I rebased #1128 so it's ready for review now as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants