BrandEmbassy interview PR #11
Conversation
.eslintcache
Outdated
| @@ -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}}} | |||
There was a problem hiding this comment.
Proč přidává github značku No newline at end of file? Musí se to dodržovat? Proč ano/proč ne?
There was a problem hiding this comment.
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
| const sortingMode = action.payload; | ||
| let orderedContacts; | ||
|
|
||
| if (sortingMode === 0) { |
There was a problem hiding this comment.
Co znamená ta 0 (dál pak i ta 1 a 2). Existuje způsob jak udělat kód čitelnější?
src/reducers/index.js
Outdated
| @@ -0,0 +1,3 @@ | |||
| /** | |||
| * Created by edgar on 19/09/2017. | |||
| */ | |||
There was a problem hiding this comment.
K čemu je tu tento soubor?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| const util = require('../src/util'); | ||
|
|
||
| /* eslint-disable */ | ||
|
|
There was a problem hiding this comment.
Proč je tu vypnutý eslint?
There was a problem hiding this comment.
protoze create react app generuje spatny JS napriklad, a znamenalo by to jen declarovat globalne komplet JEST api (pripadne Eznyme)
src/utils/util.js
Outdated
|
|
||
| isPhone(s) { | ||
| /* eslint no-useless-escape:0 */ | ||
| const regexp = /^[0-9\-\+\ ]{16}$/i; |
There was a problem hiding this comment.
Bude validace fungovat i pro string 420+111+222 - ? Jak by šlo jednoduše (automaticky) zajistit kontrolu vsupu a výstupu?
There was a problem hiding this comment.
nebude, validaci jsem opravil, puivodni v zadani byla spatne, nyni ovekava +999 999 999 999
There was a problem hiding this comment.
nerozumim dotazu: "Jak by šlo jednoduše (automaticky) zajistit kontrolu vsupu a výstupu?" kontrola obeho tam je
There was a problem hiding this comment.
@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íš.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, zeptám se ještě jinak :) Kak by šlo kontrolovat (nejlépe automaticky), že ti fce vrátí false na string 123456789?
There was a problem hiding this comment.
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
src/views/contact/ContactForm.js
Outdated
| } | ||
|
|
||
| handleCancelEdit() { | ||
| debugger; |
| this.handleSave = this.handleSave.bind(this); | ||
| this.handleDelete = this.handleDelete.bind(this); | ||
| this.handleInput = this.handleInput.bind(this); | ||
| this.validate = this.validate.bind(this); |
There was a problem hiding this comment.
Existuje ještě lepší (hezčí) způsob bindování metod objektu, dokázal bys přijít na to který?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Imho by arrow fce v create-react-app měly fungovat, ale takhle to stačí. Dík
There was a problem hiding this comment.
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!"
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :-)
| email: 'E-mail', | ||
| }; | ||
|
|
||
| export function validateInput(name, value, validationFunc, errors) { |
There was a problem hiding this comment.
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č?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A jaké hodnoty by nabývala ta proměnná errors z příkladu? Díky za odpověď
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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č?
There was a problem hiding this comment.
fieldName:false
x:false
__proto__:Object
There was a problem hiding this comment.
prvni jej naplnis validateInput('fieldName', 3, falseValidator, errors) (za predpokladu ze nezapomenes ';' nebo pouzijes standardjs :-))
podruhe errors['x'] = false;
There was a problem hiding this comment.
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č?
There was a problem hiding this comment.
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}
src/views/contact/ContactForm.js
Outdated
| this.setState({ contact: { ...this.state.contact, [name]: value }, errors }); | ||
| } | ||
|
|
||
| validate():Boolean { |
There was a problem hiding this comment.
Zde je nevalidní typ návratové hodnoty
|
PR komenty zapracovany |
sort contacts
add contact
edit contact
search contact
etc.