BrandEmbassy interview PR #11

edgar0011 · 2017-09-25T07:55:51Z

sort contacts
add contact
edit contact
search contact
etc.

jindrichsamec

Přidal jsem pár komenátřů a dotazů. Těším se na odpověď :)

Jindra S.

jindrichsamec · 2017-09-25T09:04:59Z

.eslintcache

@@ -0,0 +1 @@
+{"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/fileMock.js":{"size":43,"mtime":1506065307272,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/fileMock.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/styleMock.js":{"size":43,"mtime":1506065314353,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/styleMock.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/App.test.js":{"size":242,"mtime":1506065294606,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/App.test.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/util.test.js":{"size":509,"mtime":1506065299414,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/util.test.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/constants.js":{"size":133,"mtime":1506065243963,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/constants.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/contactActions.js":{"size":385,"mtime":1506065235084,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/contactActions.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/index.js":{"size":43,"mtime":1505856755194,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/index.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/formUtils.js":{"size":546,"mtime":1506066015463,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/formUtils.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/registerServiceWorker.js":{"size":4045,"mtime":1506067186234,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/registerServiceWorker.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/index.js":{"size":265,"mtime":1506067423592,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/index.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/contactsReducer.js":{"size":1849,"mtime":1506067383955,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/contactsReducer.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/components/contact/ContactForm.js":{"size":4152,"mtime":1506067651566,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/components/contact/ContactForm.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/views/app/App.js":{"size":4550,"mtime":1506067483953,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/views/app/App.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/util.js":{"size":484,"mtime":1506067521912,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/util.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}}}


Proč přidává github značku No newline at end of file? Musí se to dodržovat? Proč ano/proč ne?

nepridava to Github, ale GIT, je to kuli lepsimu DIFFovani souboru, kdy novy radek na konci by musel pridat vzdy
https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline/729725#729725

jindrichsamec · 2017-09-25T09:06:40Z

src/reducers/contactsReducer.js

+      const sortingMode = action.payload;
+      let orderedContacts;
+
+      if (sortingMode === 0) {


Co znamená ta 0 (dál pak i ta 1 a 2). Existuje způsob jak udělat kód čitelnější?

jindrichsamec · 2017-09-25T09:07:05Z

src/reducers/index.js

@@ -0,0 +1,3 @@
+/**
+ * Created by edgar on 19/09/2017.
+ */


K čemu je tu tento soubor?

pro propad vice reduceru, to tak delam jako konvence, v tomto pripade je zbytecny, ale planoval sem zvlast reducer na hlavni obrazovku pro sortovani a vyhedavani,
chtel jsem tim naznacit jakou delam strukturu filu

a eslint nebyl soucasti create react app setupu, pridal jsem to stejen jako flow...
obecne create react app je hodne omezujici viz babel a webpack nastaveni

jindrichsamec · 2017-09-25T11:27:56Z

src/__tests__/util.test.js

-const util = require('../src/util');
+
+/* eslint-disable */
+


Proč je tu vypnutý eslint?

protoze create react app generuje spatny JS napriklad, a znamenalo by to jen declarovat globalne komplet JEST api (pripadne Eznyme)

jindrichsamec · 2017-09-25T11:36:39Z

src/utils/util.js

+
+  isPhone(s) {
+    /* eslint no-useless-escape:0 */
+    const regexp = /^[0-9\-\+\ ]{16}$/i;


Bude validace fungovat i pro string 420+111+222 - ? Jak by šlo jednoduše (automaticky) zajistit kontrolu vsupu a výstupu?

nebude, validaci jsem opravil, puivodni v zadani byla spatne, nyni ovekava +999 999 999 999

nerozumim dotazu: "Jak by šlo jednoduše (automaticky) zajistit kontrolu vsupu a výstupu?" kontrola obeho tam je

@edgar0011 Takto: fce isPhone vrátí pro string --420+111+222+-+ hodnotu true i když to není validní telefonní číslo -> to je špatně. (u čísla výše jsou mezery navíc, které ale nejsou zřetelné) Mě by zajímalo, jak zajistíš, že se chyba nebude opakovat, až ji opravíš.

puvodni REGEX v zadani byl tento: /^\d{9}$/i;, coz je v rozporu pak s palceholderem, nahradil jsem tim nejjednodsuim po ruce, nevim zda je cilem zadani opravovat i regexp nebo spise ukazaka prace s React a Redux, code styling a structure. Pokud je to jinak prosim o upresneni, dekuji.

Ok, zeptám se ještě jinak :) Kak by šlo kontrolovat (nejlépe automaticky), že ti fce vrátí false na string 123456789?

nerozumim, automaticky? prece je to validacni fce ta se muze volat po submitu, behem psani/input eventy na input fieldu atd, takze by mela reagovat jen an vstup uzivatele...
a momentalne na string '123456789' vraci false

jindrichsamec · 2017-09-25T11:39:28Z

src/views/contact/ContactForm.js

+  }
+
+  handleCancelEdit() {
+    debugger;


Toto je tu navíc.

jindrichsamec · 2017-09-25T11:41:37Z

src/views/contact/ContactForm.js

+    this.handleSave = this.handleSave.bind(this);
+    this.handleDelete = this.handleDelete.bind(this);
+    this.handleInput = this.handleInput.bind(this);
+    this.validate = this.validate.bind(this);


Existuje ještě lepší (hezčí) způsob bindování metod objektu, dokázal bys přijít na to který?

samozrejme, ale to by nesmel byt pouzity create react app, ten omezuje pokrocile BABEL funkce jako transform-class-properties/
(facebook/create-react-app#167)
idealni je arrow zapis primo class/instance methody

Imho by arrow fce v create-react-app měly fungovat, ale takhle to stačí. Dík

nefunguji (jako class properties), a je to zamer autoru create-react-app, kuli tomu ze tyto BABEL funkce jsou uz mene zazite, mene otestovane atd:
"The point of this project is to provide good defaults (i.e. sensible, tested and solid) with a good DX. We've had a discussion about including decorators by default (#107), but the current consensus is that with the spec changing we're not ready to include them just yet.

For now, you'll have to eject to add support for them!"

V linkovaném issue je ale diskuze o dekorátorech, což je zase něco jiného. Pokud u svého PR zrušíš babel konfiguraci do původního stavu, budou ti arrow funkce fungovat :)

urcite vim ze ejectem muzu delat co chci, mohl jsem zvolit i vlastni setup kompletne, ale drzel jsem se instrukci...zadani neni dokonael (viz regexp na isPhone ..) kazdopadne zminena diskuze se tyka vse babel preset stage 2, tady i transform class properties :-)

jindrichsamec · 2017-09-25T12:00:13Z

src/utils/formUtils.js

+  email: 'E-mail',
+};
+
+export function validateInput(name, value, validationFunc, errors) {


V čem spočívá výhoda předávání objektu errors v parametru fce? Musí se vytvářet pak i proměnná newErrors? Co kdybych pak metodu zavolal tímto způsobem? (falseValidator je jen testovací validátor, který vrací vždy false)

const falseValidator = () => false let errors = {} validateInput('fieldName', 3, falseValidator, errors) errors['x'] = false;

Co bude obsahovat proměnná errors a proč?

jako pipe, flow, kde se do metody posial aktualni stav chyb, tedy je jen pto jedno formularove pole ale pro vsechny a zadna validacni funkce neni zodpovedna za stav chyb ktere se ji netykaji

cele to umoznuje vetis reuse, validate, by slo parametrizovat, nap telefon nemusi byt povinny atd., zlo by validate volat od jinud a jen pro nektere fieldy atd.

A jaké hodnoty by nabývala ta proměnná errors z příkladu? Díky za odpověď

z jakeho prikladu?

zkusim to vysvetlit:

validateInput parazituje na objektu errors, nevyrabi novy pokud existuje, a jen pridava validani flag (podle name)

mozna te mate zapis:
let newErrors = errors;
to je jen reassignemtn aby konvenoval s eslint nastavenim (AirBnB)

export function validateInput(name, value, validationFunc, errors) {
const valid = validationFunc(value);
let newErrors = errors;
if (!valid) {
newErrors = newErrors || {};
newErrors[name] = true;
} else if (valid && newErrors) {
newErrors[name] = null;
delete newErrors[name];
}
return newErrors;
}

Myslel jsem tento příklad

const falseValidator = () => false let errors = {} validateInput('fieldName', 3, falseValidator, errors) errors['x'] = false;

jaký bude obsah proměnné errors a proč?

fieldName:false x:false __proto__:Object

prvni jej naplnis validateInput('fieldName', 3, falseValidator, errors) (za predpokladu ze nezapomenes ';' nebo pouzijes standardjs :-))
podruhe errors['x'] = false;

Poslední dotaz :) Kdybych příklad upravil a výstup fce uložil do proměnné newErrors.

const falseValidator = () => false let errors = {} const newErrors = validateInput('fieldName', 3, falseValidator, errors) errors['x'] = false;

co bude obsahovat proměnná newErrors až kód doběhne? A proč?

moc nevim kam otakza miri, ale protoze je to pruchozi reference,
newError bude to same jako v predchozim pripade
newErrors === errors, mutujeme ve validateInput

{fieldName: true, x: false}

jindrichsamec · 2017-09-25T12:00:44Z

src/views/contact/ContactForm.js

+    this.setState({ contact: { ...this.state.contact, [name]: value }, errors });
+  }
+
+  validate():Boolean {


Zde je nevalidní typ návratové hodnoty

edgar0011 · 2017-09-25T12:43:23Z

PR komenty zapracovany

edgar0011 added 9 commits September 13, 2017 17:05

initial commit

2c36664

feat(): tests stub fixes

c18648c

feat(): tests runs fixes

6059a8e

feat(): setup, bable, eslint, store, reducers, app clean up

978e08c

feat(): formInput

d96566d

feat(): contacts edit, save finished

9c7dca0

git clean up

5de4d4d

feat(): clean up

134c7bb

feat(): search contacts, sorting ci

39aef44

jindrichsamec suggested changes Sep 25, 2017

View reviewed changes

refactor(): PR comments

e1085a4

fix(): phone regexp

b667540

jindrichsamec approved these changes Sep 25, 2017

View reviewed changes

feat(): clean up

fa66c0d

jindrichsamec closed this Dec 5, 2018

		@@ -0,0 +1 @@
		{"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/fileMock.js":{"size":43,"mtime":1506065307272,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/fileMock.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/styleMock.js":{"size":43,"mtime":1506065314353,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__mocks__/styleMock.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/App.test.js":{"size":242,"mtime":1506065294606,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/App.test.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/util.test.js":{"size":509,"mtime":1506065299414,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/__tests__/util.test.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/constants.js":{"size":133,"mtime":1506065243963,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/constants.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/contactActions.js":{"size":385,"mtime":1506065235084,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/actions/contactActions.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/index.js":{"size":43,"mtime":1505856755194,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/index.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/formUtils.js":{"size":546,"mtime":1506066015463,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/formUtils.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/registerServiceWorker.js":{"size":4045,"mtime":1506067186234,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/registerServiceWorker.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/index.js":{"size":265,"mtime":1506067423592,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/index.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/contactsReducer.js":{"size":1849,"mtime":1506067383955,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/reducers/contactsReducer.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/components/contact/ContactForm.js":{"size":4152,"mtime":1506067651566,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/components/contact/ContactForm.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/views/app/App.js":{"size":4550,"mtime":1506067483953,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/views/app/App.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}},"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/util.js":{"size":484,"mtime":1506067521912,"hashOfConfig":"15gt41v","results":{"filePath":"/Users/edgar/Work/Javascript2017/brandEmbassy/interview/src/utils/util.js","messages":[],"errorCount":0,"warningCount":0,"fixableErrorCount":0,"fixableWarningCount":0}}}

BrandEmbassy interview PR #11

BrandEmbassy interview PR #11

Conversation

edgar0011 commented Sep 25, 2017

jindrichsamec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgar0011 Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgar0011 Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgar0011 Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgar0011 Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

jindrichsamec Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

edgar0011 Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jindrichsamec Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

edgar0011 Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgar0011 commented Sep 25, 2017

edgar0011 Sep 25, 2017 •

edited

Loading

edgar0011 Sep 25, 2017 •

edited

Loading

edgar0011 Sep 25, 2017 •

edited

Loading

edgar0011 Sep 25, 2017 •

edited

Loading

jindrichsamec Sep 25, 2017 •

edited

Loading

edgar0011 Sep 25, 2017 •

edited

Loading

jindrichsamec Sep 25, 2017 •

edited

Loading

edgar0011 Sep 25, 2017 •

edited

Loading