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

Add coinche logic #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add coinche logic #33

wants to merge 3 commits into from

Conversation

gaut-b
Copy link
Collaborator

@gaut-b gaut-b commented Apr 7, 2020

No description provided.

Copy link
Owner

@augnustin augnustin left a comment

Choose a reason for hiding this comment

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

Good job !

Je pense que dans l'idée tu peux poursuivre sur ton implémentation, la direction est la bonne. 👍 👍

J'ai pas mal de remarques comme tu pourras le lire.

Côté UX, il manque vraiment un "A vous/XX de jouer", et un rappel de l'annonce quand on est en jeu, en plus d'améliorer le style des annonces.

Côté technique, il faudrait 2 choses :

  • que les actions soient déduites de l'état du serveur
  • probablement avoir un sélecteur côté client isCardPhase qui regarde si les 3 dernières annonces sont des passes ou une coinche pour savoir s'il faut afficher le composant d'annonce.

Enfin, je me dis que tant qu'on n'a pas officiellement validé que c'est mieux d'avoir un contrôle fort sur les règles de la coinche, je dirais que ça serait cool de pouvoir configurer l'ancien mode en option, où on ne fait que jouer le jeu de la carte. Mais je comprends que tu n'aies pas envie de bosser là-dessus, je pourrai m'y pencher à l'occase. :-)

Well done dude ! 👍 👍

@@ -0,0 +1,3 @@
<svg width="348" height="372" viewBox="0 0 348 372" fill="none" xmlns="http://www.w3.org/2000/svg">
<path d="M171.5 370.593C171.157 369.975 169.682 365.505 168.222 360.66C158.968 329.954 141.789 298.861 113.021 260.748C102.213 246.429 92.0403 233.749 64.532 200.311C32.6807 161.593 23.5571 149.19 14.3441 132.083C8.90366 121.98 3.26568 106.609 1.41034 96.82C-0.443946 87.0367 -0.474173 71.0532 1.34585 62.7305C8.74913 28.8763 38.8206 3.33577 74.7788 0.361695C115.972 -3.04531 151.011 17.7641 169.422 56.5684L173.533 65.2335L176.628 58.3881C181.225 48.2167 185.653 41.3373 193.126 32.754C212.154 10.9003 234.8 0.195349 262.081 0.157899C274.301 0.141149 281.303 1.15538 291.716 4.45044C307.29 9.37855 319.014 17.0557 329.455 29.1636C356.665 60.7169 353.443 103.704 319.923 156.351C312.103 168.632 297.995 186.803 278.593 209.582C256.25 235.814 245.332 249.146 234.155 263.848C207.364 299.088 189.345 331.108 179.434 361.091C177.922 365.665 176.338 369.927 175.914 370.562C174.929 372.037 172.312 372.056 171.5 370.593Z" fill="#FF0000"/>
Copy link
Owner

Choose a reason for hiding this comment

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

C'est plutôt M171.6, non ? 🙃

const {tableId} = useContext(LocalStateContext);
const image = isHidden ? images['BLUE_BACK'] : images[value];

const handleClick = (e) => {
const cardValue = value;
if (!isActivePlayer) {
window.alert("Ce n'est pas votre tour !")
Copy link
Owner

Choose a reason for hiding this comment

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

Ok faut que je voie ce que ça donne côté UX. Notamment ptet avoir la possibilité de forcer.

const ClubSymbol = require('../../images/clubs.svg');

const symbols = {
'H': HeartSymbol,
Copy link
Owner

Choose a reason for hiding this comment

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

Pas besoin des guillemets pour les clés.

import '../../scss/components/declaration.scss';

const INITIAL_STATE = {
objectif: 80,
Copy link
Owner

Choose a reason for hiding this comment

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

objectif c'est du français.

const DiamondSymbol = require('../../images/diamonds.svg');
const ClubSymbol = require('../../images/clubs.svg');

const symbols = {
Copy link
Owner

Choose a reason for hiding this comment

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

A refacto avec Declaration. Genre mettre dans constants.js par ex.

<label className="label">Annonce</label>
<div className="control">
<input
className="input" type="number" disabled={isDeclareDisabled} name="objectif" min={state.objectif} step="10" max="160" value={state.objectif} onChange={e => handleChange(e)}/>
Copy link
Owner

Choose a reason for hiding this comment

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

min={80} sinon on ne peut pas utiliser la flèche du bas pour descendre l'annonce.

Copy link
Owner

Choose a reason for hiding this comment

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

Ou plus exactement min={lastDeclaration.objectif + 10}

</label>
</div>

<div className="field is-grouped is-grouped-multiline is-grouped-centered" >
Copy link
Owner

Choose a reason for hiding this comment

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

L'ordre des couleurs c'est pique, cœur carreau trèfle.

<div className="content media-right has-text-centered">
<DeclarationHistory />
<p className="control">
<button className="button is-primary" name="coinche" onClick={handleClick}>Coincher</button>
Copy link
Owner

Choose a reason for hiding this comment

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

Attention, on joue "Coinche à la volée", c'est-à-dire que si quelqu'un annonce quelque chose, et qu'on ton partenaire passe, c'est trop tard pour coincher. A prendre en compte. 😄

onChange={e => handleChange(e)}
className="form-check-input"
/>
Standard
Copy link
Owner

Choose a reason for hiding this comment

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

Je suis pas sûr qu'il y ait besoin de la notion de "standard". En gros les "couleurs" possibles c'est :

  • Pique
  • coeur
  • carreau
  • trèfle
  • tout atout
  • sans atout

// If currentPlayer made a declaration and everybody else passed OR somebody coinched
// launch the game
if (((currentDeclaration.playerId === currentPlayer.id) && isActivePlayer) || (currentDeclaration.isCoinched)) {
launchGame(tableId);
Copy link
Owner

Choose a reason for hiding this comment

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

Hum ok ça y est je comprends.

En soit, c'est pas totalement déconnant que ce soit le client qui déclenche ces actions, mais c'est pas très redux style.

Normalement, le joueur ne fait que ANNOUNCE, et c'est le game qui calcule l'état du jeu et donc savoir si on est encore en phase d'annonce ou phase de cartes.

Du coup, il faudrait que l'action ANNOUNCE côté serveur, fasse les checks que tu fais là. Eg. si 3 passes, return startGame();

Si nos actions étaient asynchrones, on aurait besoin de qqch comme redux-thunk pour que nos actions puissent appeler d'autres actions, mais en l'occurrence tout est synchrone donc il suffit de rerouter les actions je pense.

Si tu vois pas comment ça peut fonctionner, pas grave, j'essaierai de me pencher dessus pour faire un squelette. En attendant, ta version fera le job.

@augnustin
Copy link
Owner

augnustin commented Apr 17, 2020

Yo,

Bien ouej franchement ya grave de taf !

Je note ici les remarques que je me suis faites à la lecture de ton code. Ca ne veut pas forcément dire qu'il faut changer immédiatement, mais c'est une manière plus propre à mon sens, donc à l'occasion ça peut valoir le coup de changer quand l'occasion se présente.

  • Pour moi on n'a pas besoin de stocker teams dans le state. Même s'il n'y a pas vraiment de duplication car on ne stocke que les Id, si ceux-ci sont amené à changer (ce qui est le cas quand on a un rechargement de page par exemple), ça ne va plus fonctionner correctement.

A la place, j'aurais volontiers utilisé les indexes de players, et stocker dans quelque chose comme odd et even pour savoir de quelle équipe on parle. Si j'ai bien suivi, teams ne sert qu'à stocker le score, non ? Du coup, j'aurais volontiers utilisé l'attribut score du state qui me semble approprié.

Donc qqch comme:

score: {
  odd: [80, 80],
  even: [0, 130]
}
  • De la même manière, je ne comprends pas bien l'intérêt de stocker currentDeclaration et declarationsHistory: ne peut-on pas tout simplement considérer que currentDeclaration = last(declarationHistory) ?

Là aussi, j'ai l'impression qu'il y a une divergence entre l'un et l'autre dans ton code: par exemple si c'est coinché, on le trouve dans currentDeclaration mais pas dans declarationHistory ce que je trouve "dangereux".

  • J'ai l'impression que l'action DISTRIBUTE et NEW_GAME font plus ou moins la même chose, je serai d'avis de les refactorer.

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