jniemann66 / react-chessdiagram

Chess Diagram React Component
MIT License
26 stars 10 forks source link

Pgn viewer #7

Closed svangordon closed 7 years ago

svangordon commented 7 years ago

Implements a PGN viewer. What was <Chessdiagram> is now <BoardContainer>, and <Chessdiagram> is a top level component wrapping <BoardContainer> and <GameHistory>.

<GameHistory> is enabled by passing <Chessdiagram> a boolean prop gameHistory. Right now it supports parsing chess PGNs by default, but using the custom callbacks getAllFen, getHeader, getMovetext, getRows and getResult it should be able to accept any kind of game notation. getAllFen returns an array with all FEN positions from a PGN and is doing most of the heavy lifting.

A few more words about splitting out <BoardContainer>: <BoardContainer> is still responsible for capturing user input, etc. It's pretty much the same, I just thought that some separation of concerns would be appropriate.

jniemann66 commented 7 years ago

Wow. You put a lot of work into this one. I'm just on holidays at the moment, so only getting to computer sporadically. So, just to clarify, there are no breaking changes in 's interface ? Cheers, Judd

svangordon commented 7 years ago

Oh no worries!

There shouldn't be any breaking changes -- it still takes a fen prop, the PGN viewer isn't displayed by default, and the demo from before this PR works with the new code. It might make sense to move this to a dev branch or something like that to give it a whirl without having to worry about breaking anything for users.

Enjoy your holidays!

jniemann66 commented 7 years ago

Hi Stephen,

I'm only just getting around to approving your pull request now. Sorry it has taken so long this time ! ... been flat out with other projects and was also AFK a lot during holidays.

PGN viewer is an awesome idea, but I was wondering if it should be a different component entirely. On the other hand, integrating a PGN viewer into the one chessdiagram module would make it VERY useful for people to use in chess blogs etc for demonstrating games. So, on the balance, I think it's all good.

As an aside, for a while I have been wanting to somehow implement server-side Winboard and UCI chess interfaces to host chess engines, but this would definitely be an entirely different abstraction layer and in a separate module. (I have a chess engine written in C++ that could run on a node server etc.)

Cheers, Judd

jniemann66 commented 7 years ago

Merged. Thanks very much for this. I like the "Game of the Century" game in the demo ! Nice work.

jniemann66 commented 7 years ago

Stephen, I have found a few breaking changes. I haven't had time to get to the bottom of them yet, but:

  1. It now crashes if user doesn't supply a callback function for onMove (Callback shouldn't be mandatory)
  2. In my Knight's Tour app (which uses the Piece@Square notation, with the pieces prop), it seems to be rendering the start position only, and ignoring whatever I put into the 'pieces' prop. It is just trying to render this:
              <Chessdiagram
                  squareSize={this.refs.diagramContainer ? Math.min(80, 0.7 * this.state.diagramDimensions.width / this.state.boardWidth) : 45}
                  files={this.state.boardWidth}
                  ranks={this.state.boardHeight}
                  pieces={positionDescriptor}
                  onMovePiece={this._movePiece.bind(this)}
                  onSelectSquare={this._selectSquare.bind(this)}
                />  
  3. In the demo, the PGN viewer seems to be overriding the static FEN functionality, and it is now ignoring the FEN input (also breaking draughts and courier chess)
jniemann66 commented 7 years ago

OK, I have fixed these things now. (See issues #8, #9, #10 - all closed)

Cheers, Judd