Analysis/study page keyboard input #15019

brollin · 2024-04-02T10:15:17Z

Let's add the keyboardMove input to the analysis and study page. Not sure that the normal location directly under the board is best, but I implemented it there for the time being:

schlawg · 2024-04-02T15:37:29Z

I'm really pumped about this. Especially if you're interested in adding voice after! It's been requested several times. Variants may be tricky.

brollin · 2024-04-03T01:16:22Z

Totally! Hoping to get voice in there right after. And I also have expectations of bug reports with both given the complexity of analysis+study+variants O_O

schlawg · 2024-04-04T16:07:37Z

ui/analyse/src/ctrl.ts

  }

-  mainlinePathToPly(ply: Ply): Tree.Path {
+  mainlinePlyToPath(ply: Ply): Tree.Path {


Lol good catch!

Couldn't unsee it 🙃

…ate agree

brollin · 2024-04-06T02:01:15Z

Okay it seems to be wired up correctly, ~~with only one very questionable bit of hackery~~ (fixed now). As for placement, now that we have something to look at maybe we can collectively decide where it should go. There are some outstanding styling issues that I didn't want to spend to much time on in case we decided on a different location.

ui/analyse/src/ground.ts

* master: (258 commits) remove more dependencies remove deps to security WIP complete permission migration to lila.core.perm move permission list to lila.core.perm provide sample issues for puzzle cli example fix table formatting New Crowdin updates (lichess-org#15015) Fix hub -> core path in bin/trans-dump Fix element name don't dox poster info in forum post search results fix Playban bus event, simplify playban api give enough space to crazyhouse pocket grantable wip fix fide monitoring remove deps to lobby & relation remove oauth dependency to user refactor UserId functions remove last shutup dependency add missing LightUser.Me conversion remove gathering dep to user ...

ornicar · 2024-04-08T15:10:48Z

ui/analyse/src/ctrl.ts

+    this.sendMove(orig, dest, capture, prom);
+  };
+
+  auxUpdate = (fen: string) => {


I know this comes from ui/round, but still, what does that even mean "auxUpdate"? It was bad then and there, and it's bad here and now.

Why do we need it, and can it get a helpful name?

I believe the intention was "auxiliary update", but agreed even spelled out it could be improved. Its purpose is to fan out updates to the various plugins, and I think it existing at all will make more sense when we implement voiceMove. We could name it something like updatePlugins?

pluginUpdate and pluginMove sound good.

At the risk of getting too in the weeds, curious why pluginUpdate vs updatePlugins? At a glance, I wouldn't necessarily know that the first is performing an action. Maybe it's returning a "plugin update" object or something. The latter is more unmistakably an action an my head, and follows the verb object order of an English sentence.

Very light preference for me here, just want to understand how you think about it.

In any case, made that naming change here: 10b0d24

As for pluginMove, something like handlePluginMove makes the most sense to me. If I'm in a controller and see that function, I know that we are handling a move that's coming from the plugin. I'm always reminding myself which way the data is flowing, and in theory this could help with that.

the actor followed by the verb is a convention shared by rounds, puzzles, and chessground. i.e userMove, apiMove, etc.

but i don't care at all, i'm the one who chose auxMove so just shut me in a soundproof closet and pick something - it's fine.

That's fair, consistency is important. Too light of a preference to break with consistency or shove anyone in a closet I think. Side note, I've loved seeing the refactored controllers (functions->classes) and overall the code is in a great spot it feels like, a nice departure from my work's codebase 😌

ornicar · 2024-04-08T15:12:22Z

ui/analyse/src/ground.ts

+            fen: ctrl.node.fen,
+            canMove: true,
+            cg: ctrl.chessground,
+          });


ctrl sub-ctrls should be instanciated within the ctrl, not in a view.

That definitely feels better. I ended up mimicking how we do things in ui/round: fcbf24f

schlawg · 2024-04-09T03:03:27Z

ui/analyse/tsconfig.json

    { "path": "../chess/tsconfig.json" },
    { "path": "../common/tsconfig.json" },
    { "path": "../game/tsconfig.json" },
+    { "path": "../keyboardMove/tsconfig.json" },


The tsc dependency mechanism isn't documented anywhere so I'm impressed. I think you're the first to do this.

I won't pretend to understand it, but I will pretend to be quite good at pattern matching 😎

it's actually straightforward. tsc doesn't care about our monorepo dependencies (package.json), so if you want correct behavior when type checking - you have to either compile everything in the correct order (bad) or express dependencies in tsconfig.json (better, but still not great).

brollin · 2024-04-14T07:22:19Z

Good catches. I fixed the up and down arrow behavior in 1a4004b.

It turns out that the computer evaluation toggle was the next thing in the tab order, and when it is focused, or any input element for that matter, mousetrap key bindings are not respected. I solved this two ways:

I changed the tab order in 06b84be such that the keyboard input was after all of the right hand side tools (ceval, move list, controls, etc) in the tab order. Now, the under board is focused after the keyboard input.
I allowed toggling the computer evaluation via enter in 7f4517e, only when it is focused. Now if that element happens to be focused and enter is pressed, something happens at least. Since it is an input, it will still be swallowing lots of other key presses (like the arrows).

I tried to move the keyboard input such that the next thing in the tab order would be the move list, but the move list is in the middle of renderTools, and the keyboard input is rendered alongside those tools. Basically the hierarchy doesn't easily allow that. Open to changing the tab order around more if desired.

brollin · 2024-04-14T07:27:52Z

ui/keyboardMove/src/main.ts

+        ArrowLeft: -1,
+        ArrowRight: 1,
+      };
+      root.userJumpPlyDelta?.(arrowKeyToPlyDelta[arrowKey]);


There's an opportunity here to just remove root.userJumpPlyDelta in favor of root.handleArrowKey. I was debating whether it would be cleaner or not 🤔

* master: (117 commits) Update LameNameTest.scala increase specificity of tv grid layout, was overridden by round remove "shit" from lamename move `object Tellable` back to lila.common final class fix unrelated typo in comment support empty study chapter player name - closes lichess-org#15185 remove 'pool' from boards.scss too fix old dasher langs style bug remove dasher .category Fix insights aggregation by opening move dasher reset button so that its text is visible, move board css to board.css hack around dasher board pref debounce only reset non-default board settings refactor dasher board defaults remove pool.webp board image due to low quality remove unused import refactor code dup, add explicit typing refactor common.Form study list pages shouldn't be full-screen ...

ornicar · 2024-05-01T14:35:45Z

It's looking a bit wrong on /analysis

Also I couldn't get it to appear on a game analysis page?

schlawg · 2024-05-01T15:08:35Z

Should we put it up on lichess.dev or testy and let some power users see how well it integrates with the existing keyboard navigation?

* master: Add missing "gameOver" i18n key to puzzles why so lazy rename lila.app.UiEnv ui tweaks AuthUi rename lila.web.ui move Controller.action complete lila.coach.ui lila.coach.ui only hide dasher board reset when actually resetting

ornicar · 2024-05-02T05:57:43Z

yes, it's on lichess.dev now

brollin · 2024-05-02T09:11:08Z

Just catching back up, I can look into why it isn't on the analysis page anymore as well as the styling.

How should I log into lichess.dev? Is there a preexisting user I can use or some way to register? Attempting to register doesn't send me an email at the moment.

brollin · 2024-05-02T09:28:48Z

Turns out there is an error when rendering the keyboard input before the underboard, but it displays fine after it (and in other positions in the vdom children array). Not sure why that error is happening, the message isn't making much sense to me: Uncaught DOMException: Node.insertBefore: Child to insert before is not a child of this node

brollin · 2024-05-02T10:18:44Z

Okay I think it's in a better state now for testing

ornicar · 2024-05-02T14:45:48Z

Uncaught DOMException: Node.insertBefore: Child to insert before is not a child of this node

This error usually means something changed the dom that snabbdom manages.

* master: Update _app.scss more puzzle ui fix lichess-org#15194 page/snippet tweaks page & snippet, it compiles only render Page and Snippet WIP lila.event.ui more user ui lila.web.ui.DevUi extra space lila.setup.ui lila.challenge.ui continue ui refactoring lila.storm.ui more analysis ui

…lila into analysis-keyboard-input * 'analysis-keyboard-input' of https://github.com/brollin/lila: one column styling fixes optionally include keyboardMove styling on user analysis page fix rendering bug with tab order change

ornicar · 2024-05-02T14:54:10Z

Deployed to lichess.dev!

* master: New Crowdin updates (lichess-org#15198) tweak site-header-margin to fully render the analysis board top player - closes lichess-org#14953 invalidate study chapter preview cache after a broadcast reset - closes lichess-org#15135 lila.report.ui more ui refactor always more ui refactor lila.relay.ui move forms to their modules fixed external link urls better fix for Bus.sub match errors hackfix Bus.sub match errors fix monitoring warning remove 4 new boards due to bad image compression fix typos (lichess-org#15196) New Crowdin updates (lichess-org#15190) Zero[A => A] add panels for placing extra pieces hide glyphs when computer analysis is disabled

ornicar · 2024-05-03T09:45:09Z

ready to go?

brollin · 2024-05-03T10:00:44Z

I think so! I'll keep my eyes out for any bug reports.

ui/analyse/css/_layout.scss

ui/analyse/css/study/_layout.scss

after #15019

brollin added 8 commits March 28, 2024 21:53

keyboardMove preference honored on analyze page

5f2dd8b

first pass at keyboard move on analyzed page

dd7573f

code golf

f5663ad

more keyboard move functionally

03a4439

implement keyboard move on study page

269431f

rename function for clarity

d899200

remove unused sendMove keyboardMove function

e677a01

clear keyboardMove input when no opponent and who command used

4a83260

schlawg reviewed Apr 4, 2024

View reviewed changes

brollin added 7 commits April 5, 2024 00:07

implement crazyhouse keyboardMove functionality on analysis page

5cfc135

remove console log

c5b80c1

ensure that keyboardMove updates only occur when controller and cg st…

e11dc11

…ate agree

augment keyboardMove root data

7da7fd8

do another pass on css

b550956

include keyboardMove in tsconfig

17bf59c

sbt scalafmt

d47eca7

brollin marked this pull request as ready for review April 6, 2024 02:01

brollin commented Apr 6, 2024

View reviewed changes

ui/analyse/src/ground.ts Outdated Show resolved Hide resolved

brollin and others added 4 commits April 7, 2024 16:34

retrieve crazyhouse pockets dynamically

4298eb9

remove unnecessary ?? ''

6f17c16

and another one

42bbde4

ornicar reviewed Apr 8, 2024

View reviewed changes

schlawg reviewed Apr 9, 2024

View reviewed changes

brollin added 2 commits April 9, 2024 00:07

move creation of keyboardMove into controller

fcbf24f

extract helper function for clarity

a5558c1

brollin added 2 commits April 14, 2024 00:03

put keyboard move towards end of tab order

06b84be

allow toggling ceval via enter when focused

7f4517e

brollin commented Apr 14, 2024

View reviewed changes

schlawg and others added 3 commits April 28, 2024 17:41

fix merge conflicts

862a8d5

remove duplicated code probably coming from a merge

eda9c79

only hide dasher board reset when actually resetting

104f937

Merge branch 'master' into analysis-keyboard-input

bad525a

* master: Add missing "gameOver" i18n key to puzzles why so lazy rename lila.app.UiEnv ui tweaks AuthUi rename lila.web.ui move Controller.action complete lila.coach.ui lila.coach.ui only hide dasher board reset when actually resetting

fix rendering bug with tab order change

b32612e

brollin added 2 commits May 2, 2024 02:36

optionally include keyboardMove styling on user analysis page

812f0c7

one column styling fixes

650176c

ornicar added 2 commits May 2, 2024 16:46

ornicar added 2 commits May 3, 2024 11:43

fix previous merge

a31f327

ornicar reviewed May 3, 2024

View reviewed changes

ui/analyse/css/_layout.scss Show resolved Hide resolved

ornicar reviewed May 3, 2024

View reviewed changes

ui/analyse/css/study/_layout.scss Show resolved Hide resolved

ornicar merged commit c4d18da into lichess-org:master May 3, 2024

ornicar added a commit that referenced this pull request May 9, 2024

fix analysis ZH layout on mobile - closes #15248

a21a45b

after #15019

Uh oh!

Analysis/study page keyboard input #15019

Analysis/study page keyboard input #15019

Uh oh!

Conversation

brollin commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schlawg commented Apr 2, 2024

Uh oh!

brollin commented Apr 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brollin commented Apr 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ornicar Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brollin commented Apr 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ornicar commented May 1, 2024

Uh oh!

schlawg commented May 1, 2024

Uh oh!

ornicar commented May 2, 2024

Uh oh!

brollin commented May 2, 2024

Uh oh!

brollin commented May 2, 2024

Uh oh!

brollin commented May 2, 2024

Uh oh!

ornicar commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ornicar commented May 2, 2024

Uh oh!

ornicar commented May 3, 2024

Uh oh!

brollin commented May 3, 2024

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

brollin commented Apr 2, 2024 •

edited

Loading

brollin commented Apr 6, 2024 •

edited

Loading

ornicar Apr 8, 2024 •

edited

Loading

ornicar commented May 2, 2024 •

edited

Loading