Skip to content

Conversation

@cyanfish
Copy link
Member

@cyanfish cyanfish commented Jun 13, 2024

Deeply nested variations (in studies etc.) can cause visual clutter that make it difficult to navigate through complex trees.

This PR includes some baseline visual changes to the left-side "branch" to improve clarity (before, after):

On initial load deep variations are now collapsed (depth 3, 5, 7, etc. are collapsed by default). Clicking the + or navigating to the parent move will expand the variations out.
image

You can also expand or collapse variations from the context menu:
image

When you do this from the context menu it will operate recursively. So for example if you right-click the first move of the game, it effectively acts as "collapse all" or "expand all". Or you can right-click on a specific variation to just collapse that variation and its siblings.

cyanfish and others added 8 commits June 11, 2024 17:39
- Remove the "head" rendered as part of the <lines> element as it doesn't accurately represent where we're branching from
- Remove the "tail" that extends past the bottom variation as it's visually unnecessary. This means the height is no longer the same as the parent height, so we need a new <branch> element with variable height.

Tested on Firefox & Chrome with inline & column mode in various zoom levels.
The node's "collapse" flag we now treat as a tri-state where "undefined" means to use the default collapse state.
We've added "position: relative" to the <line> elements for styling purposes, but that affects the calculation of the "offsetTop" property. We can instead calculate the offset ourselves.
}

lines.collapsed > * {
display: none;
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of hiding them, can we avoid rendering them altogether? And maybe get some perf gains in deep trees.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

return;
}
cont.scrollTop = target.offsetTop - cont.offsetHeight / 2 + target.offsetHeight;
const targetOffset = target.getBoundingClientRect().y - el.getBoundingClientRect().y;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we now require 2 calls to getBoundingClientRect?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put a note in the "Fix autoscroll offset" commit, let me copy it here:

We've added "position: relative" to the <line> elements for styling purposes, but that affects the calculation of the "offsetTop" property. We can instead calculate the offset ourselves.

@ornicar
Copy link
Collaborator

ornicar commented Jun 14, 2024

Looking great.

@ornicar ornicar merged commit 387c00b into lichess-org:master Jun 14, 2024
@Siderite
Copy link

Thanks for the heads up.

@Siderite
Copy link

You have to test it better, though. It's buggy.
Example: 1. e4 e5 (1... d5 2. a4 a5) (1... f5 2. d4 d5) 2. h4 h5 *
No matter which node you collapse on (e5, d5 or f5) the e5 main line remains visible and the other two get collapsed.
Also, if you collapse something into a + sign, how do you know what you want to expand later?

@cyanfish
Copy link
Member Author

Yes, that's intended. Main lines are not collapsible. And variations expand/collapse as a group, which is certainly debatable but it keeps things simple.

@steptro
Copy link
Contributor

steptro commented Jun 15, 2024

This is great!

ornicar added a commit that referenced this pull request Jun 17, 2024
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.

4 participants