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.

Sign up for GitHub

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

Jump to bottom

Refactor and reactive-config prop feature #190

Merged
merged 20 commits into from
Jun 14, 2023
Merged

Refactor and reactive-config prop feature #190

merged 20 commits into from
Jun 14, 2023

Conversation

gavin-lb
Copy link
Contributor

@gavin-lb gavin-lb commented Jun 13, 2023 •
edited
Loading

Made the following changes:

  • Fixed config merging (now does a deep merge/copy and returns a new object instead of mutating arguments)
  • If movable.events.after is supplied in config, patch changeTurn to run before it rather than replace it entirely
  • Added setConfig method to the API
  • Overall refactor to combine redundant operations and move logic away from Vue component
  • Cleaned up PromotionDialog to be less convoluted
  • Fixed old tests for new internal API structure
  • Added optional config reactivity. Defaults to false to preserve backwards compatibility as new behaviour may be surprising if implementation doesn't expect it
  • Added tests for new features
  • Updated docs
  • Added premove support (didn't realise it would just take 1 line lol)

I understand this is quite a big PR and I plan on continuing development so feel free to let me know if you'd rather I just fork the repo instead! 👍

Also very new to typescript and frontend stuff in general, so any and all feedback very much welcomed :)

Planned changes:

  • Add events for: threefold repetition, insufficient material and game over (which returns which type of game over)
  • Fix enpassent animation
  • Fix promotion dialogue click through (if one of your own pieces is behind the selected promotion piece, it will be selected accidentally)
  • Maybe split API into 2 classes? 1 for public exposed API functionality and 1 for private handling of board and game
  • Premove support Edit: added!
  • Game history viewer
  • Fix free move mode
  • Maybe obscure config from user? ie. move entirely to props and emits for more idiomatic Vue component behaviour
  • Possibly engine integration?

gavin-lb added 14 commits June 8, 2023 23:19
@gavin-lb
removed unneccessary ref clutter
5664452
@gavin-lb
formatting: fix quotes
8420080
@gavin-lb
Changed merge to be a pure function that returns a new deepcopied obj…
7123710 
…ect rather than mutating the args
@gavin-lb
passthrough of the userdefined movable.events.after function
5150c26
@gavin-lb
changed config merge function to properly deepcopy (structuredClone d…
254d94c 
…oes not work for functions)
@gavin-lb
cleaned up initialisation of config and board
faaf953
@gavin-lb
fixed movable.events.after patching
f1ec4c2
@gavin-lb
Big refactor, simplified promotion, added setConfig API method
55842b7 
 - Refactored a lot of code to combine redudunant operations and move logic away from Vue component. Was able to remove lots of code from API methods by consolidating it into other methods.
 - Cleaned up the process for getting a promotion (now just sends it back via a callback function) and improved the PromotionDialog component.
 - Added a set config method to the API with the intention of making it possible to have a reactive config that watches the config prop for changes and updates the board config as neccessary.
@gavin-lb
fixed broken test (clearly bxa1 is a black pawn move, not white)
776e0b4
@gavin-lb
updated eslint rules
3dcf1d1 
Turned off 'vue/require-explicit-emits' rule as this seems to be a Javascript rule that does not work well to Typescript. Other linter rules already flag undeclared emits (so this rule is redundant) and it does not understand the Typescript generic type arguement way of specifying emit type (so it is unhelpful).
@gavin-lb
Added new reactive config feature
7825f44 
It can be turned on as a prop and defaults to false to preserve backwards compatibility as new behaviour may be surprising if implementation doesn't expect it
@gavin-lb
fixed setConfig patching of movable.events.after is user provided und…
7cc7244 
…efined
@gavin-lb
added tests for new features
0298118
@gavin-lb
updated docs for new features
8a99a98
@gavin-lb
changed test formatting to appease github-code-scanner
5ee33ad 
The previous test was perfectly valid formatting, but the scanner doesn't seem to understand it. Perhaps this will be better?
@qwerty084
Copy link
Owner

Thanks for your pr. This looks very good. I will take a closer look at it tomorrow.

Pull request are very welcome, you can definitely keep developing here if you want to.

gavin-lb added 4 commits June 13, 2023 21:51
@gavin-lb
added premove support and some simplifications
6cb5eeb
@gavin-lb
Removed movable.color switching
510e06f 
I discovered that Chessgrounds only allows the color whose turn it is to move when `movable.color` is set to 'both', so theres actually no need to switch it back and forth between 'white' and 'black' manually every turn.

This kind of makes the `player-color` prop a bit pointless, as it turns out to just be a proxy for a config value. Instead of removing it I've kept it for backwards compatibility and simply merge its value into the config during `setConfig`.
@gavin-lb
Reverting previous commit
29321f1 
Actually there is a minor difference in functionality between the two approaches. When using 'both' you can't move the non-turn players pieces, but you can pick them up. Whereas the previous functionality was the pieces could not even be picked up. Reverting commit for consistency.
@gavin-lb
Removing props.playerColor merging in setConfig
7201bda 
Forgot to remove this while reverting previous commit, sorry for the commit spam!
qwerty084
qwerty084 approved these changes Jun 14, 2023
View reviewed changes
Copy link
Owner

@qwerty084 qwerty084 left a comment

Choose a reason for hiding this comment

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

Great work! Thank you.

@qwerty084 qwerty084 merged commit ed0b7d0 into qwerty084:main Jun 14, 2023
@qwerty084
Copy link
Owner

I will include this pr in v1.2 release. I will release an alpha version soon, for testing. But we can try also include your other planned changes in it.

@qwerty084
Copy link
Owner

I've created a pre-release with your changes.

npm i [email protected]

@gavin-lb
Copy link
Contributor Author

Hi, thanks for including my changes. Sorry I've not made another PR with any of the planned changes yet, I was implementing the threefold repetition event and found a bug in chess.js, so I fixed it and made a PR and that kind of lead me down a chess.js rabbit hole making more changes.

Once I've finished with some chess.js changes I'll be back to work on the above planned changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwerty084 qwerty084 qwerty084 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gavin-lb @qwerty084