Skip to content
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.

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

add option to show multiple games from a PGN #31

Closed
wants to merge 12 commits into from
Closed

add option to show multiple games from a PGN #31

wants to merge 12 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 23, 2023

Also takes away the option to set the FEN as an option (it can still be set with an explicit [FEN "..."] tag on the PGN, though).

Here is a screenshot! (I took the PGN from here: https://lichess.org/study/ZglxzC5N)

screenshot

@@ -2,7 +2,6 @@ import { Opts } from './interfaces';

const defaults: Opts = {
pgn: '*', // the PGN to render
fen: undefined, // initial FEN, will append [FEN "initial FEN"] to the PGN
Copy link
Contributor

Choose a reason for hiding this comment

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

gotta keep that somehow

Copy link
Contributor

Choose a reason for hiding this comment

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

let's just keep the current behavior of adding the fen to the first game.

@ornicar
Copy link
Contributor

ornicar commented Oct 23, 2023

please stop force-pushing after the PR is published, as I'm committing to it too.

src/pgn.ts Outdated
@@ -135,5 +135,6 @@ function makeMetadata(headers: Headers, lichess: Lichess): Metadata {
isLichess: !!(lichess && site?.startsWith(lichess)),
timeControl,
orientation: orientation === 'white' || orientation === 'black' ? orientation : undefined,
name: event ?? '?',
Copy link
Contributor

Choose a reason for hiding this comment

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

in a broadcast, all games will have the same event. Probably better to try with the player names first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to display an index (+1) in the select options, to always allow distinguishing the games, whether they have distinct names or not.

src/pgnViewer.ts Outdated
if (index !== this.selectedGame) {
this.selectIndex(index);
if (to === 'first') path = Path.root;
if (to === 'last') path = this.game.pathAtMainlinePly('last');
Copy link
Contributor

Choose a reason for hiding this comment

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

first and last here are weird. They'll jump to the first or last game, but only if we're at the first or last ply of the current game. That doesn't seem intuitive.

@ornicar
Copy link
Contributor

ornicar commented Oct 23, 2023

looking good, almost there!

@ornicar
Copy link
Contributor

ornicar commented Oct 24, 2023

A9Y5yBo

the new select breaks several layouts.

I doubt it can be made to work in every configuration. But, if we move it into the menu, then there shouldn't be any problem.

@ghost ghost closed this by deleting the head repository Aug 19, 2024
This pull request was closed.
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.

2 participants