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

Chess example with n4js and react + tutorial #1

Merged
merged 22 commits into from May 10, 2019

Conversation

Projects
None yet
4 participants
@qtran-n4
Copy link

commented May 8, 2019

Chess example with n4js and react + tutorial

Note that: Redux is NOT used in this PR yet.

Read the README of the feature branch (https://github.com/NumberFour/n4js-tutorial-chess/tree/n4js-tutorial-chess) for instruction on running the app as well as generating the tutorial page html locally.

@qtran-n4 qtran-n4 requested review from mmews-n4, mor-n4 and halitogunc May 8, 2019

@qtran-n4

This comment has been minimized.

Copy link
Author

commented May 8, 2019

optional @dbo in case you are interested :D

@@ -0,0 +1,17 @@
#!/bin/bash

This comment has been minimized.

Copy link
@dbo

dbo May 8, 2019

this deserves an NPM task

This comment has been minimized.

Copy link
@qtran-n4
export public function Square(props: SquareProps): React.Element<?> {
let backgroundColor: string = props.isWhite? '#EADAB9' : '#C2A482';
backgroundColor = props.isPicked? '#00ff00' : backgroundColor;
backgroundColor = props.isValidDestination? 'yellow' : backgroundColor;

This comment has been minimized.

Copy link
@dbo

dbo May 8, 2019

this above is not easy to understand, simple if statements would help

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Thanks. Improved using if statements

*/
public getBoardCoordinateRepresentation() {
const rowLabel = 8 - this.row;
const colLabel = String.fromCharCode(97 + this.col);

This comment has been minimized.

Copy link
@dbo

dbo May 8, 2019

maybe better "a".charCodeAt(0)

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

yeah better. Done.

path: path.resolve(__dirname, 'public/dist')
},
devtool: 'inline-source-map',

This comment has been minimized.

Copy link
@dbo

dbo May 8, 2019

mixed single and double quotes

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Thanks. Fixed.

"version": "0.0.1",
"scripts": {
"start": "webpack-dev-server",
"build": "webpack"

This comment has been minimized.

Copy link
@dbo

dbo May 8, 2019

misses a build step

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Added build step.

@mmews-n4

This comment has been minimized.

Copy link

commented May 8, 2019

General:

  • I noticed the shell script: What should Windows users do?
  • Why do we have the two files index.html and chess.html that have the same content?
  • Also, we have chess.adoc which has a different content than chess.html. I understand that form chess.adoc the file n4js-tutorial-chess.html is generated. Would it make sense to rename chess.adoc to n4js-tutorial.adoc then?

Regarding the file README.adoc:

  • Maybe start with one or two sentences saying what this project is about?
  • Developer Setup
    • "Step 2: The project is already an Eclipse >|< with..." [A word seems to be missing]
    • "Step 4: Clean & Build the project. After this step, the project should has no..." [have]
  • Build the tutorial page
    • "To simplify >|< thing a bit" [A word seems to be missing]
    • "Move to the root folder" ['Move' is a little unclear since it is usually used when referring to moving folders or files. I think you mean change directory? Mind the second occurrence in your text.]
@qtran-n4

This comment has been minimized.

Copy link
Author

commented May 8, 2019

@mmews-n4

  1. I noticed the shell script: What should Windows users do?
    No worries. The shell script build-tutorial.sh is supposed to used by us (IDE-team) to generate the tutorial on a Mac/Linux machine. The end user is more interested in starting the application via npm start which should be identical on all machines.

  2. Why do we have the two files index.html and chess.html that have the same content?
    Good point. I have removed chess.html and copy public/index.html and use it instead during build tutorial.

  3. Also, we have chess.adoc which has a different content than chess.html. I understand that form chess.adoc the file n4js-tutorial-chess.html is generated. Would it make sense to rename chess.adoc to n4js-tutorial.adoc then?
    Done. Renamed the Asciidoc file to n4js-tutorial.adoc.

  4. README
    Improved README according to your feedback.

@mmews-n4
Copy link

left a comment

I noticed that some files have a read sign at the end saying, that a newline is missing at the end.

/**
* Get all opponent pieces
*/
private getpiecesOfSameColor(squares: Array<Array<Piece>>, white: boolean): Array<~Object with {piece: Piece, pos: Coordinate}> {

This comment has been minimized.

Copy link
@mmews-n4

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Done.

return result;
}

private calculateBishopAttackingDestinations(squares: Array<Array<Piece>>, piece: Piece, pos: Coordinate): Array<Coordinate> {

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

Regarding all calculate***AttackingDestinations(...) methods: Did you consider to merge these methods into one and factoring the deltas out?

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Pieces such as bishops, pawns, queens do share common behavior/implementation. But knights pawns are quite different. I would say: leave it as exercise for the reader 🙈

height: '100%'
}

export public class LabelSquare extends React.Component<CoordinateSquareProps, Object> {

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

Maybe this exported class should also get some js-doc? (like above)

This comment has been minimized.

Copy link
@qtran-n4
const squares = new Array<React.Element<?>>();
let isWhiteInitial = true;
// Draw a b c ... h
squares.push(this.renderLabelSquare(''));

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

Indentation seems odd

This comment has been minimized.

Copy link
@qtran-n4
== Overview

link:https://reactjs.org/[React] is a popular JavaScript library created by Facebook widely used for developing web user interface. N4JS provides full support for React as well as the JavaScript extension link:https://reactjs.org/docs/introducing-jsx.html[JSX] for describing UI elements. Internally, we have been using N4JS in combination with React and JSX for years to develop very large e-commerce web applications.
In this tutorial, we will develope a simple and yet fun (!) chess game with N4JS and React. For the purpose of this tutorial, the chess game only allows two humans to play against each other.

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

typo: develope

This comment has been minimized.

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Sry fixed.

The chess web app is served at port 8080.

== Developer Setup

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

You could link from the file README.adoc to this section. This would remove the copy of this section in the README file.

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

I will leave it that way as that would be a bit complicated.

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

I think it is just <<doc/n4js-tutorial-chess.adoc>> which would link to the other file. Linking to a section in another file would be <<doc/n4js-tutorial-chess.adoc#developer_setup,Developer Setup>>`. The id/anchor is "developer_setup" which is the generated name. You can set the id/anchor manually using:

[[DeveloperSetup]]
== Developer Setup

In order to enable N4JS's type checking for React, we need to declare `@n4jsd/react` as a dev dependency in `package.json` of the project. `@n4jsd/react` consists of `n4jsd` files that contain file definitions for React.

[source,n4js]

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

This is a json formatted code snippet.

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Fixed. Done

[source,n4js]
----
{
"name": "n4js-tutorial-chess",

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

Why do you include this code snipped here?

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Removed. Done.

Instead of images, we use link:https://en.wikipedia.org/wiki/Chess_symbols_in_Unicode[Chess symbols in Unicode] to display chess pieces.
There are many ways to represent pieces in N4JS, the simplest way being to declare a link:https://www.eclipse.org/n4js/spec/N4JSSpec.html#_string-based-enums[String-based enum] called `Piece` to represent pieces. The enum literals are the Unicode chess symbols.

[source,typescript]

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

Above, you used n4js as code formatting option. Why do you use typescript here?

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

we have to use typescript because PRISM does not understand n4js. Our tutorials so far do not have syntax highlighting...

/**
* A history snapshot
*/
interface ~Snapshot {

This comment has been minimized.

Copy link
@mmews-n4

mmews-n4 May 8, 2019

How about briefly telling the reader what the '~' is about? (Structural vs nominal typing, relation to TypeScript, why is it used here, ...)

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 8, 2019

Author

Added explanation of structural subtyping. Done.

@mmews-n4

This comment has been minimized.

Copy link

commented May 8, 2019

Cool to have this intense tutorial! 👍

@halitogunc

This comment has been minimized.

Copy link

commented May 8, 2019

Lock files are missing.

Show resolved Hide resolved public/index.html Outdated
Show resolved Hide resolved src/components/Square.n4jsx Outdated
public getBoardCoordinateRepresentation() {
const rowLabel = 8 - this.row;
const colLabel = String.fromCharCode('a'.charCodeAt(0) + this.col);
return `(${rowLabel + ',' + colLabel})`;

This comment has been minimized.

Copy link
@halitogunc

halitogunc May 9, 2019

suggestion: getBoardCoordinateRepresentation and toString methods return similar strings but the way of their implementation is different. i.e:

const str1 = `${variable},`;
const str2 = `${variable + ','}`;

It would be more understandable if we use same structure either blocks.

This comment has been minimized.

Copy link
@qtran-n4

qtran-n4 May 10, 2019

Author

getBoardCoordinateRepresentation convert (row,column) coordinates into board coordinates according to the algebraic chess notation. toString simply returns (row,column).

This comment has been minimized.

Copy link
@halitogunc

halitogunc May 10, 2019

Yes, you are right but I am not talking about what they are doing. My point is that they are using template literals in the end but in different ways. If we use the same style in the return statement like ${variable}, or ${variable + ','} then it would be more understandable. Anyway it was just a suggestion. :bowtie:

Show resolved Hide resolved src/main.n4jsx Outdated
Show resolved Hide resolved src/Move.n4js Outdated
@qtran-n4

This comment has been minimized.

Copy link
Author

commented May 10, 2019

Lock file added.

@halitogunc

This comment has been minimized.

Copy link

commented May 10, 2019

It is nice to have a good in-depth tutorial.
Thanks! 👏 🎉 @qtran-n4

@qtran-n4

This comment has been minimized.

Copy link
Author

commented May 10, 2019

Thanks for the review guys!

@qtran-n4 qtran-n4 merged commit 6895d11 into master May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.