Settings page #9

OriLev · 2018-02-03T06:12:29Z

It's still a work in progress, including some ideas about changing up some structures (for example passing whatever goes under <PageMain> as children so that PageMain will become even dumber and can be used strictly for structure design purposes.

Expected future additions:

Validation of user input and default input values for 'no input'.
Adding feedback on user input validation.
Adding more design to the settings page.

testing git

preparing for first review

BFS algorithm and path drawing added

Fix props and functions naming in buttonGrp

* after ESLint * add ESLint * use destructuring in App and fix most Lint errors in App.js.js * break up the components into separate folders

git merge --abort

* fix Nick's comments to PR#7 * fix a function name according to Nick's comments to PR#7 * fix Nick's comments to PR#7 and PR#8 * remove dist/ from git repository * add page header to settings page * add color inputs and submit button for settings page * separate AppButton component

elektronik2k5 · 2018-02-05T06:10:48Z

app/components/ColorInput/ColorInput.js

+  };
+  return (
+    <div className="inputWrapper">
+      <label htmlFor={ `newColor${colorLetter}` }>{`color ${colorLetter}:`}


You don't need an id on an input and a for attribute on a label if the former is nested in the latter. Like here :)
It just works and that's the only way I write input labels, because it avoids dealing with ids and their uniqueness constraint.

It's the ESLint...

looking for the rule I need to bend to my will so it doesn't give me the error here.

done

elektronik2k5 · 2018-02-05T06:12:50Z

app/components/GameInstructions/GameInstructions.js

  return (
    <div>
-      { currentInstructions }
+      <InstructionsBox { ...instructionsProps } />


When you've only got a prop or two, extracting them only adds cognitive load instead of reducing it. I'd inline it.

done

Frankly, I was trying to keep a unified style - so that always are some kind of props passed into an element via { ...*some*Props }

but I do agree that if there are is only one prop, no point on making things more complicated

elektronik2k5 · 2018-02-05T06:17:38Z

app/components/NewColorsInput/NewColorsInput.js

+      colorLetter,
+      currentNewColor,
+      defaultText,
+      key: index,


Assuming the colors are unique, they would make a better index. Read about keys in react docs.

Indeed a good point. I automatically dismissed all the other data since I didn't have any id passed in, but the colorLetter will be a great key in this case.

elektronik2k5 · 2018-02-05T06:22:15Z

app/components/SettingsPanel/SettingsPanel.js

+  updateNewColor(e, colorLetter) {
+    const colorUpdate = {};
+    colorUpdate[`newColor${colorLetter}`] = e.target.value;
+    this.setState(colorUpdate);


ES2015 version is a one liner:

this.setState({ [`newColor${ colorLetter }`]: e.target.value, })

that's a very interesting syntax I haven't seen before

Thanks! :)

elektronik2k5 · 2018-02-05T06:26:44Z

app/components/SettingsScreen/SettingsScreen.js

+import PageMain from '../PageMain/PageMain';
+
+export default function SettingsScreen({ setBoardColor, }) {
+  const headerProps = { instructions: [ 'Please enter the colors of your choice in hex format' ], };


UX: "Please" is rarely used nowadays in microcopy texts. It doesn't add any value.

elektronik2k5 · 2018-02-05T06:27:47Z

app/components/SubmitColorsButton/SubmitColorsButton.js

+  const linkProps = {
+    className: 'btn',
+    to: { pathname: '/', },
+    children: 'Submit new colors',


UX: "Submit" is not a verb normal people use. When did you ever submit in real life last time? :)

elektronik2k5 · 2018-02-05T06:28:26Z

app/style.css


 .btn, .success{
+  text-decoration: none;
 	color: white;


Inconsistent indentation

elektronik2k5 · 2018-02-05T06:29:14Z

app/style.css

@@ -102,6 +124,7 @@
 }

 .btn, .success{


selector1, selector2, selectorN { // rule }

You can add stylelint to enforce this and other rules.

good idea, will have to wait until I have time to read about it and how to use it properly more.

* add validation on input entry * add validation for new color inputs * clean SettingPanel variables * add feedback and design and design the settings panel * change the color input type from text to color * add reset button * prepare for github pages deployement

OriLev and others added 16 commits January 10, 2018 03:37

Merge pull request #1 from OriLev/basic

4b07bc1

testing git

for reinitialization

1f05f25

Merge branch 'master' of https://github.com/OriLev/PathTracker

f2656c0

additional preparation

4b6485c

Merge pull request #2 from OriLev/basic

689eea5

preparing for first review

Merge branch 'basic'

f0fcee8

after merge with basic

0fdb5a6

Merge branch 'master' of https://github.com/OriLev/PathTracker

3d52e8a

Merge pull request #3 from OriLev/basic

a5de9f0

BFS algorithm and path drawing added

Merge pull request #4 from OriLev/basic

54341d9

Fix props and functions naming in buttonGrp

separate components to folders and start fixing coding style (#5)

23527f4

* after ESLint * add ESLint * use destructuring in App and fix most Lint errors in App.js.js * break up the components into separate folders

add ESLint configuration

de14f27

Merge branch 'master' of https://github.com/OriLev/PathTracker

35c4b3d

git merge --abort

Merge branch 'basic'

2ee89e7

add page header to settings page

7621394

add color inputs and submit button for settings page

f5e034b

OriLev requested a review from elektronik2k5 February 3, 2018 06:13

OriLev assigned OriLev and elektronik2k5 Feb 3, 2018

OriLev and others added 2 commits February 4, 2018 00:02

fix missing key in NewColorsInput

97d1b18

elektronik2k5 requested changes Feb 5, 2018

View reviewed changes

OriLev and others added 7 commits February 10, 2018 19:26

change lint rule regarding the label tag and refactor code accordingly

45a6d9b

update key value from index to colorLetter

b4dbd1b

fix indentation

74b9a73

refactor the setState syntax

6c35250

change text shown

7cdf415

Merge branch 'master' into settings-page

17a41fe

Settings page #9

Are you sure you want to change the base?

Settings page #9

Uh oh!

Conversation

OriLev commented Feb 3, 2018

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!

OriLev Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

OriLev Feb 10, 2018 •

edited

Loading