ArneVogel / listudy

Listudy - chess training server
https://listudy.org
GNU Affero General Public License v3.0
292 stars 45 forks source link

Comments option #135

Closed olleeriksson closed 1 year ago

olleeriksson commented 1 year ago

I am just creating this draft PR for you to be able to give your opinion before I clean it up. So I will discard it and put up an updated one when I'm done. It has not language files, and some commented code, and I haven't tested it fully, but if you want to try it feel free to go ahead.

I called the option "Comments: only played move" and "Comments: move + responses". Thoughts?

As you can see below I managed to add a bit of space between comments. I think that make it much easier to tell them apart. I also moved the comment from the move that was just played to the top, and the comments from the coming available responses from the player below. I also made the text size of the comments 0.9em instead of 1.0em. The underlines move names are taken from the code programmatically. It only shows on comments from the responses you have available. The rest is text from the comment itself:

image

Here is another example where the user is showing only comments from the move that has been played.

image

And here is one with even longer text:

image

It also occurred to me that these two icons (marked with an arrow) could be used if one wants to have the icon flip between two different states when changing the comments option:

image

olleeriksson commented 1 year ago

If it helps: here is a study with a few moves from the Sicilian opening with some comments: https://lichess.org/study/InuSjp1l

And the same repertoire imported into LiStudy: https://listudy.org/en/studies/4l9p9e-sicilians-with-comments https://listudy.org/en/studies/oahxer-sicilians-with-comments-black

I've started the comment on every move with "White|Black plays xxxx" so it's easy to tell where each comment comes from.

ArneVogel commented 1 year ago

I added the icons.

Some thoughts:

  1. I think having the "White|Black plays xxxx - " added automatically would be nice.
  2. Since we already know the move for which we display the comment, how about making the move interact able: When you hover (or click on mobile) show an arrow showing the move on the board.
  3. Going even further, do the same with the moves in the actual comment. If it is a legal move in the current position make it interact able as in point 2.
olleeriksson commented 1 year ago

Alright, I'll see what I can do..

olleeriksson commented 1 year ago

Ok, so I tried to do what you suggested, and I refactored the code a little to prepare for adding things like being able to click the move to make it, but I couldn't get the onclick handler to work because of browser security issues. I think maybe the most useful thing would be highlighting the move with an arrow, but I didn't want to start implementing that in the middle of everything. I think I have to limit myself now. I've spent so many late nights that I need to wrap it up a bit. But it should be an interesting future thing to add.

So I left it like this, and the PR is ready for review now. I did listen to your comment number 1 with a slight modification. Here is a screenshot of how it looks now. I put in a newline between the move and the comment, and added "White|Black resonse [Move]". I skipped the word "response" or "played" for the move that was just played, because I felt it helped make it stand out a little. Also, if you're only showing comments from the played move, I am not even adding the color "Black|White". I felt it wasn't needed then.

If you still want me to use the wording that you suggested or make any changes of course I'll do that. But I will leave highlighting the arrow and being able to click on the move for a later time.

Oh and BTW, I already have the "Options in localstorage" code ready, but I'll wait to upload it until this one has gone through in case there are any code conflicts.

Anyway, a sample:

image

Showing comments only from the played move:

image

olleeriksson commented 1 year ago

It's ready now.

olleeriksson commented 1 year ago

Oh and I don't know how you prefer to merge things, but there are a couple of commits this time, so if you prefer the straight line type of history it might be an idea to do a squash merge. Anyway, just so you're aware of it.

ArneVogel commented 1 year ago

Something I just noticed: there is currently no indication if a comment is cut off: No indication of scroll: image

When there is text below: image

Just noting for further improvements. After a short investigation I am not sure if its possible just using css so this would probably have to get solved in js.

olleeriksson commented 1 year ago

Yeah I noticed that large comments don't behave great, but I didn't want to start making too many modifications either. But something should be done to handle large amounts of text in the comments.

On Sat, Nov 26, 2022, 10:38 ArneVogel @.***> wrote:

Something I just noticed: there is currently no indication if a comment is cut off: No indication of scroll: [image: image] https://user-images.githubusercontent.com/26939450/204082043-a5be5e11-cbd0-4c35-b45e-e04dbc2e9533.png

When there is text below: [image: image] https://user-images.githubusercontent.com/26939450/204082054-e2b171c8-d91b-45a3-b750-1ac59c6d2034.png

Just noting for further improvements. After a short investigation I am not sure if its possible just using css so this would probably have to get solved in js.

— Reply to this email directly, view it on GitHub https://github.com/ArneVogel/listudy/pull/135#issuecomment-1328014610, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJZZQQHWAO4J732NWOSUMNTWKHK7RANCNFSM6AAAAAASIOC65Y . You are receiving this because you authored the thread.Message ID: @.***>

ArneVogel commented 1 year ago

Some issues I found while exploring comment options (from https://lichess.org/study/Jn2JyhRT/6nJ8VQoA)

Comments unattached to any more before the game bugs out, and opening moves should probably not be a response image

ArneVogel commented 1 year ago

Could you please add the comment that would be placed before the study back? As with the comment above just without the "Black NaN...undefined:". Otherwise looks good to me.

olleeriksson commented 1 year ago

Alright. There you go!

ArneVogel commented 1 year ago

Thanks a lot!