Skip to content

make conditional premoves clickable #12788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Jun 15, 2023
Merged

make conditional premoves clickable #12788

merged 12 commits into from
Jun 15, 2023

Conversation

@TheTrio
Copy link
Contributor

@TheTrio TheTrio commented May 4, 2023 •
edited
Loading

This PR Attempts to fix #12707

Looks like this

out-39-21.mp4

I'm not really sure about the styling but everything else seems to work fine.

TheTrio added 2 commits May 4, 2023 18:43
@TheTrio
make conditional premoves clickable
6db8dea
@TheTrio
remove unnecessary redraw
ac65283 
this fixes the issue where in clicking a premove after updating it still shows the old premove
kraktus
kraktus reviewed May 4, 2023
View reviewed changes
Copy link
Member

@kraktus kraktus left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR!

Couple points

Currently the jump does not work when the nodes are not already present in the analysis board. This is an issue because conditional moves lines are saved across moves and devices. Probably just create the nodes then.

Nice to have, instead of jumping to the end of line allow the user to choose which move to jump. Could be done in a separate/follow up PR if needed. I think it should be made by editing renderNodesHtml to allow an optional event-listener.

If you have any questions feel free to ask here or on discord

TheTrio added 2 commits May 5, 2023 03:21
@TheTrio
remove hardcoded path
da38e0d
@TheTrio
attach to click listener
5fcb74b 
before, the event listener was only being rebound when a new premove was added - but it wasn't being rebound when a premove was edited.

This caused it to use the old value of the premove, which was incorrect. This fixes that.
@TheTrio
Copy link
Contributor Author

TheTrio commented May 5, 2023

Currently the jump does not work when the nodes are not already present in the analysis board. This is an issue because conditional moves lines are saved across moves and devices. Probably just create the nodes then.

I'm not sure I follow. Could you explain in terms of an example?

@TheTrio
Copy link
Contributor Author

TheTrio commented May 5, 2023 •
edited
Loading

I've also used the click event listener instead of the hook now - this is because bind was adding the event listener manually and so I would have to remove the event listener manually before adding a new one(which I need to do whenever nodes is updated)

This handles that on its own(as far as I can tell anyway)

@TheTrio TheTrio requested a review from kraktus May 5, 2023 18:02
@kraktus
Copy link
Member

kraktus commented May 5, 2023

Currently the jump does not work when the nodes are not already present in the analysis board. This is an issue because conditional moves lines are saved across moves and devices. Probably just create the nodes then.

I'm not sure I follow. Could you explain in terms of an example?

Sure
image
if you click in this situation jump works fine

image

But if you clear the moves manually (or in the practice the user will be using another device)

image

Then click again on the line this time it does not jump. It should instead create the node in the analysis board

@TheTrio TheTrio force-pushed the makeForecastsClickable branch from 0696283 to 4736545 Compare May 5, 2023 19:06
@TheTrio
Copy link
Contributor Author

TheTrio commented May 5, 2023 •
edited
Loading

Thanks for explaining that! I believe I have a fix for this. I'm not sure about hardcoding children to [] though. I assume that it gets updated by the helper?

@TheTrio TheTrio force-pushed the makeForecastsClickable branch from 4736545 to e8f9b2e Compare May 5, 2023 19:20
@TheTrio
Copy link
Contributor Author

TheTrio commented May 29, 2023

@kraktus Hi - been a while. Just pinging to see if this can be merged now

@kraktus
Copy link
Member

kraktus commented May 29, 2023

Hey, ornicar has the final call and has been very busy these last weeks, he will review when he can 👍

@TheTrio
Copy link
Contributor Author

TheTrio commented May 30, 2023

Sure. Just wanted to see if there was an update. Thanks!

ornicar added 3 commits June 15, 2023 14:50
@ornicar
Merge branch 'master' into makeForecastsClickable
6940780 
* master: (752 commits)
  fix default translation - closes lichess-org#13034
  fix puzzle board menu on mobile - closes lichess-org#13028
  remove coach review translations
  log oauth prompts
  make the mobile oauth authorization page less scary
  fix oauth with signed token
  more controller code golf
  controller code golf
  delete json export
  no credentialless on Plan.index
  live setting for credentialless UA matcher
  try disabling credentialless
  New Crowdin updates (lichess-org#13022)
  +Artsakh Republic flag lichess-org#11631
  ~Fix daily puzzle ui jumping when toggling lichess-org#12960
  given Conversion[scalatags.Text.Frag, Fu[Result]]
  export reviews as json
  refactor LilaController
  support show moves as a voice command
  log unsigned oauth bearers
  ...
@ornicar
no need add a new snabbdom module
1671a02
@ornicar
move forecast logic from view to ctrl
15d20c2
@ornicar
Copy link
Collaborator

ornicar commented Jun 15, 2023

nice work!

@ornicar ornicar merged commit fb7e332 into lichess-org:master Jun 15, 2023
@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 15, 2023 •
edited
Loading

Thanks @ornicar! I'm not sure why when I used the bind hook, it was referring to the old value of the premove - which is the reason why I added an event listener in the first place

Would you have any ideas? Either ways, thanks again!

@TheTrio TheTrio deleted the makeForecastsClickable branch June 15, 2023 13:13
@ornicar
Copy link
Collaborator

ornicar commented Jun 15, 2023

uh, I don't quite understand, sorry. From my tests it seems to work as intended?

@ornicar
Copy link
Collaborator

ornicar commented Jun 15, 2023

please get the latest master as I just pushed some forecast refactoring

@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 15, 2023

I was actually talking about 5fcb74b which is when I moved from bind to using the eventlistener module - which you reverted just now.

I don't remember it working when I used bind, but it seems to be working fine now after your change - which is what confused me. Oh well - doesn't matter now I suppose - all's good. Sorry for bothering - I was just curious.

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

Reviewers

@kraktus kraktus kraktus approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature request: Clicking on conditional premove on correspondence chesss analysis page to preview it.

3 participants

@TheTrio @kraktus @ornicar