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

Truncate Score Display #300

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Ekatwikz
Copy link

@Ekatwikz
Copy link
Author

Ekatwikz commented May 15, 2024

Adding a couple things:

  • score formatting: changed truncation to be optional once again (still using lila's way though, as some components would overflow regardless)
  • score bubble: increased width to 4.5rem, and disabled truncation

});

test("should format a large cp score correctly", () => {
expect(formatScore({ type: "cp", value: 12345.67 })).toBe("+99.0");
Copy link

@Disservin Disservin May 16, 2024

Choose a reason for hiding this comment

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

a) centipawn values are only integers
b) while the conversion might be useful for lichess it isn't so much for a local gui, Stockfish displays values up to 20000 for centipawns and we have a nice relation between the distance to zero for tablebase position official-stockfish/Stockfish@def2966 it be unfortunate to lose this information.

Copy link
Author

Choose a reason for hiding this comment

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

a) Good point, I'll change that test to use a sensible value. I'm still learning the engine interface stuff as I go
b) Afaik, the truncated values will only be used on the eval bar, which is too narrow to display arbitrarily sized values without overflowing. The exact value of the eval bar is displayed by a tooltip

Choose a reason for hiding this comment

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

b) ah okay if it is only the evalbar then it is fine, I had the fear that it might be reused for the engine evaluation display

Copy link
Author

Choose a reason for hiding this comment

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

I just double checked, and there is also some truncation (in my branch, but also upstream) on the summary to the right of the engine component that displays the evaluation of the first variation
The space here is wide enough to use the full value without truncation, I can change it to do that if that's preferred too

to display possible 5 digit cp values without overflowing
precision is more common throughout
@@ -66,22 +66,31 @@ function ScoreBubble({
textAlign: "center",
padding: "0.15rem",
borderRadius: theme.radius.sm,
width: size === "md" ? "4rem" : "3.5rem",
width: size === "md" ? "5.0rem" : "3.5rem",

Choose a reason for hiding this comment

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

Are these changes from 4rem to 5rem really necessary? I think it would be better to change it to minWidth and keep it at 4rem

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.

3 participants