fix Nick's comments to PR#7 #8

OriLev · 2018-01-27T23:06:50Z

No description provided.

elektronik2k5

Getting better

elektronik2k5 · 2018-01-28T02:34:47Z

.eslintignore

-dist
+node_modules/
+dist/
 app/utils/BFS.js


Remove. Replace with a comment inside the file itself.

elektronik2k5 · 2018-01-28T02:35:37Z

.eslintrc.js

+        objects: "always",
+        imports: "always",
+        exports: "always",
+        functions: "always-multiline"


Trailing comma? If your conf is JS, you should lint it too :)

elektronik2k5 · 2018-01-28T02:38:29Z

app/components/ButtonGroup/ButtonGroup.js

-            <button { ...buttonProps }>
-              {text}
-            </button>
+            <button { ...buttonProps } />


Now that it's a single line expression, drop the parents: return <button {...buttonProps} />

done.

now, this section looks not very clear to my eyes at the moment. I mean, this structure of a div containing a relatively long map() inside of it.

I was thinking about taking out the function inside the map and putting it prior to the component's return.

you'll see it in the next commit. let me know what you think

elektronik2k5 · 2018-01-28T02:39:15Z

app/components/Cell/Cell.js

    pathEndingPoint,
-  } = state;
+  },
+  cellIndex: { x, y, },


Not actually "index". Coordinates?

elektronik2k5 · 2018-01-28T02:41:51Z

app/components/GameButtonsPanel/GameButtonsPanel.js

  const isGoButtonDisabled = (locationOfStart, locationOfEnd) => (
    !isStartOrEndButtonDisabled(locationOfStart) || !isStartOrEndButtonDisabled(locationOfEnd)
  );
+  const getButtonClasses = isButtonPressed => `btn${isButtonPressed ? ' pressed' : ''}`;


OK, time for you to look into BEM naming. Don't necessarily use BEM, but learn about it and it's ideas and it will help you reason better about CSS/HTML class names.

elektronik2k5 · 2018-01-28T02:44:26Z

app/components/Instructions/Instructions.js


 export default function Instructions({ state, }) {
-  function buildInstructionArray(appState) {
+  function createInstructionArray(appState) {


Plural "Instructions"

elektronik2k5 · 2018-01-28T02:45:44Z

app/components/Instructions/Instructions.js

      `Press on the "${buttonText}" button in order to go back to toggle mode`
    );
-    const setReferencePoint = buttonText => (
+    const getReferencePoint = buttonText => (


The line above ends with "Text", but this one doesn't. Be consistent. Decide how to name all similar parts and follow your decision.

done.
Thought adding the word Text at the end of the variables below.
Then felt it would be a useless repetition.

Now, if I could put them all inside an object called instructions, but then I would destructure it and all the magic of using instructions['toggleTiles'] would go away. Is there sort of a middle ground?

instructionTexts?

But what about the retrieval of the strings? Wouldn't it be problematic with the DRY principle that I would use instructionTexts several times?

Destructure it?

elektronik2k5 · 2018-01-28T02:46:37Z

app/components/Instructions/Instructions.js

-        instructionList.push(gameInstructions.goToChooseEndMode);
+        instructionList.push(goToChooseEndMode);
      }
      if (pathStartingPoint.length !== 0 && pathEndingPoint.length !== 0) {


DRY all .length === 0 into named consts. It will make it much more readable too. The code will say what, not how.

elektronik2k5 · 2018-01-28T02:48:15Z

app/components/Instructions/Instructions.js


 export default function Instructions({ state, }) {
-  function buildInstructionArray(appState) {
+  function createInstructionArray(appState) {


"Array" is an implementation detail. The caller shouldn't know it.

elektronik2k5 · 2018-01-28T02:50:51Z

app/components/Instructions/Instructions.js

-      instructionList.push(gameInstructions.chooseReferencePoint);
-      instructionList.push(gameInstructions.goBackToToggleTileMode);
+      instructionList.push(chooseReferencePoint);
+      instructionList.push(goBackToToggleTileMode);


All these pushes can be DRYed and made readable: const addInstruction = instructionList.push. You may need to use bind too though: const addInstruction = instructionList.push.bind(instructionList)

elektronik2k5 · 2018-02-03T02:05:56Z

app/components/ButtonsGroup/ButtonsGroup.js

+  const style = {
+    height: `${100 / buttons.length}%`,
+  };
+  function createButton(buttonObject, index) {


Function returns a react component, so it should adhere to the naming convention: start with a Capital and not a verb. I know it's confusing, since it is also a function. But in JS most things are functions, and the function naming convention is a good baseline, which has exceptions for some cases. Some of these exceptions are in plain JS, others are for frameworks.
Examples:

Before ES2015, classes were functions, so function Animal(){} is a thing, as long as that's a class.

Factories (which return instances of different classes), are also functions and start with a Capital: function Vehicle(), which can return a Car or Boat or Spaceship.

React stateless components :)

so in this specific case Button() would make sense?

would it also require moving it to a separate file..? (cause then I suspect that Button() would be a bit too general)

Yes, if inside the file, you can use Button. I'd call it Some Button and extract to a new file. GroupButton? AppButton?
If generic, Button is good. If specific, should be "SomeSpecificButton".

elektronik2k5

I think this is enough changes for this PR :)
Start making more focused changes in feature branches now.

fix Nick's comments to PR#7

7aa2942

OriLev requested a review from elektronik2k5 January 27, 2018 23:07

OriLev assigned OriLev and elektronik2k5 Jan 27, 2018

fix a function name according to Nick's comments to PR#7

3d11c26

elektronik2k5 reviewed Jan 28, 2018

View reviewed changes

OriLev requested review from elektronik2k5 and removed request for elektronik2k5 February 1, 2018 16:13

OriLev added 4 commits February 2, 2018 00:17

fix Nick's comments to PR#7 and PR#8

94c8c15

remove dist/ from git repository

f8ec057

add page header to settings page

7621394

add color inputs and submit button for settings page

f5e034b

elektronik2k5 reviewed Feb 3, 2018

View reviewed changes

elektronik2k5 approved these changes Feb 3, 2018

View reviewed changes

OriLev and others added 2 commits February 4, 2018 13:43

separate AppButton component

c988be9

Merge branch 'master' into basic

bc5aaa0

OriLev merged commit c5e4335 into master Feb 4, 2018

fix Nick's comments to PR#7 #8

fix Nick's comments to PR#7 #8

Uh oh!

Conversation

OriLev commented Jan 27, 2018

Uh oh!

elektronik2k5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elektronik2k5 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants