Skip to content

Improve brush type #245

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 3 commits into from
Jan 22, 2023
Merged

Improve brush type #245

merged 3 commits into from
Jan 22, 2023

Conversation

qwerty084
Copy link
Contributor

@qwerty084 qwerty084 commented Jan 15, 2023

brush in DrawShape is currently typed as string | undefined.
Passing undefined to brush crashes when using setShapes(): Uncaught TypeError: brush is undefined (svg.ts:87)
So it should be atleast string type.

I also added the available colors to the brush property, from state.ts. Because if you pass black as a string it just crashes. It would be nice if the available colors are in the type.

@niklasf
Copy link
Member

niklasf commented Jan 15, 2023

Thanks!

Uncaught TypeError: brush is undefined (svg.ts:87)

Somewhere along the way the possibility of brush being undefined must have been ignored/lost, or else this error should have been caught at compile time. Let's fix that, too.

@qwerty084 qwerty084 requested a review from niklasf January 16, 2023 23:05
@niklasf niklasf merged commit 80f1c63 into lichess-org:master Jan 22, 2023
@qwerty084 qwerty084 deleted the improve-brush-types branch January 22, 2023 16:51
niklasf added a commit to niklasf/chessground that referenced this pull request Jan 22, 2023

Verified

This commit was signed with the committer’s verified signature.
niklasf Niklas Fiekas
A particular small set of brushes (green, red, blue, yellow) can be selected
for drawing while holding various keys. These are mandatory.

By default, some more brushes are also available (paleGreen, paleRed, paleBlue,
paleGrey) for programmatic usage.

All can be overwritten, and more can be configured, so long as the 4
mandatory ones exist.
@niklasf
Copy link
Member

niklasf commented Jan 22, 2023

Sorry for the delay. Merged now.

I also refreshed my understanding of the drawing code, and tweaked it a bit more: 8eef51c.

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.

None yet

2 participants