Skip to content

feat: allow scaling arrows #38

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

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Conversation

tom-anders
Copy link
Contributor

I'd need this for implementing different arrow sizes in the mobile app, based on the engine evaluation of each move.

This is a really naive approach of course, but it will probably be alright for starters? We might want to apply a different scaling to the arrow body vs its tip in the future (Looks like lichess web UI does that as well).

@veloce
Copy link
Collaborator

veloce commented Jul 3, 2024

Yes the scale should apply to the arrow body and tip at the same time, even for a first version I'd say. Shouldn't be too hard to do I suppose?

@tom-anders
Copy link
Contributor Author

Yes the scale should apply to the arrow body and tip at the same time, even for a first version I'd say. Shouldn't be too hard to do I suppose?

This PR already does that - I meant that we might want to allow different scales for body and tip in the future

@tom-anders
Copy link
Contributor Author

Yes the scale should apply to the arrow body and tip at the same time, even for a first version I'd say. Shouldn't be too hard to do I suppose?

This PR already does that - I meant that we might want to allow different scales for body and tip in the future

(line 79 scales the body, line 94 scales the tip)

@tom-anders
Copy link
Contributor Author

Sorry, I should've included screenshots from the beginning to make things clearer, here's an arrow with scale=0.5:

smallArrow

and here's one with scale=1.5:

bigArrow

@veloce
Copy link
Collaborator

veloce commented Jul 4, 2024

Why would we want to scale differently the body and the tip? No it's fine like that.

In your screenshot I see a thin white separation between the rectangle and the triangle. I don't think the current implementation has this issue. In any case, could you check that? Thanks.

@tom-anders
Copy link
Contributor Author

tom-anders commented Jul 4, 2024

Why would we want to scale differently the body and the tip? No it's fine like that.

In your screenshot I see a thin white separation between the rectangle and the triangle. I don't think the current implementation has this issue. In any case, could you check that? Thanks.

Yeah you're right, it at first looked to me like the old lichess app uses different scalings there, but at a closer look that was just my brain playing tricks.

Will take a look at the separation issue!

@tom-anders
Copy link
Contributor Author

tom-anders commented Jul 4, 2024

In your screenshot I see a thin white separation between the rectangle and the triangle. I don't think the current implementation has this issue. In any case, could you check that? Thanks.

Ok I tested this again, and the main branch has the same issue (see below). This is probably because I took the screenshot from a Linux desktop build, not my phone.

Screenshot_2024-07-04_12-48-53

@veloce
Copy link
Collaborator

veloce commented Jul 4, 2024

If the problem is only on linux, that's fine. Can you please send a screenshot from an emulator or device with the scaling to show it's fine? Thanks.

@tom-anders
Copy link
Contributor Author

Heres a screenshot from lichess-org/mobile#828, doesn't look like it has the issue. Let me know if you want a screenshot of the example app running on Android instead.

@veloce
Copy link
Collaborator

veloce commented Jul 4, 2024

No that's fine, thanks.

Copy link
Collaborator

@veloce veloce left a comment

Choose a reason for hiding this comment

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

Can you please solve the conflicts and put the new version? Then I merge it. Thanks.

@tom-anders
Copy link
Contributor Author

All done, thanks for the review!

@veloce veloce merged commit 67714ad into lichess-org:main Jul 5, 2024
1 check passed
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