Week 4 #6

apedroferreira · 2019-04-07T12:23:58Z

No description provided.

dannytce

Great job guys! :)

I found only a couple of things.

One thing regarding API - could you please create api/customers folder? :)

dannytce · 2019-04-08T13:11:55Z

.eslintrc.js

@@ -25,5 +25,6 @@ module.exports = {
    'prefer-named-capture-group': 0,
    'react/no-did-mount-set-state': 1,
    'react/prop-types': 1,
+    'no-shadow': [2, { allow: ['name'] }],


Kudos for not disabling the whole rule, but just allowing what needs to be allowed 💪

dannytce · 2019-04-08T13:12:26Z

.stylelintrc

@@ -1,5 +1,6 @@
 {
 "extends": [
-    "@strv/stylelint-config-styled-components"
+    "@strv/stylelint-config-styled-components",
+    "stylelint-config-prettier"


I didn't know about it 😅 Thanks for educating :D

dannytce · 2019-04-08T13:17:20Z

src/components/Form/index.js

+  display: flex;
+  flex-direction: column;
+  margin: 14px auto;
+  max-width: 100%;


Shouldn't be this opossite? :) max-width: 420px; and width: 100%

Both work but yeah that makes more sense.

dannytce · 2019-04-08T13:18:15Z

src/components/Input/index.js

+  <Wrapper>
+    <Label htmlFor={name}>{label}</Label>
+    <StyledInput
+      hasError={!!error}


Please use Boolean operator instead of !!. It's much easier for our students to follow.

dannytce · 2019-04-08T13:26:25Z

src/pages/SignUp/index.js

+  renderSuccess() {
+    return (
+      <Layout>
+        <H1 textAlign="center">{`You've signed up!`}</H1>


It could be just a string, no need to for template literals

This is because there was an ESLint warning about the apostrophe. Would you prefer if I used &#8217? Or just disable the rule?

Ah, ESLint... ok than :)

dannytce · 2019-04-08T13:26:48Z

src/pages/SignUp/index.js

+            values,
+          }) => (
+            <Form onSubmit={handleSubmit}>
+              {!!globalError && (


Same here, please use Boolean operator.

dannytce · 2019-04-08T13:35:02Z

src/pages/SignUp/index.js

+                type="text"
+                label="First name"
+                value={values.firstName}
+                onChange={handleChange}


You are missing per Input onBlur event too. This leads to unexpected behavior with error messages (because when navigation accross input, touched wouldn't happen).

So either fix it by adding onBlur everywhere. Or go easily with Field from formik :) Btw, have you considered using it? Or why you actually didn't use it in the first place? :)

I thought about it but wanted to reduce complexity. Also thought the error messages aren't perfect. Will try to improve it and use Field ;)

dannytce · 2019-04-08T13:38:05Z

src/api/fetch-api.js

+export const fetchAPI = async (url, options) => {
+  const token = await getAccessToken()
+
+  const response = await fetch(`${config.apiUrl}/${url}`, {


There is extra / in url, because other api calls have it in the beginning :) Pick one of them, but current solution. is not working

dannytce · 2019-04-08T13:42:05Z

src/App.js

@@ -16,7 +18,8 @@ class App extends Component {
          <GlobalStyles />
          <Switch>
            <Route path="/" exact component={ProductList} />
-            <Route path="/cart" component={Cart} />
+            <PrivateRoute path="/cart" component={Cart} />


For demonstration purposes let's rather create a new route. Let's say "My Profile". Don't need to implement it (add there a comment, that students can do that by them selfs if want to).

But anyway, "privatization" of Cart could lead into more discussions (aka it's possible to add product to cart without beiing logged in, but not seeing the cart... etc.). So let's use the simpliest version, which could be My Profile :P

apedroferreira · 2019-04-09T10:17:35Z

@dannytce Added a commit with code review fixes and some other, let me know if it's all good!

apedroferreira · 2019-04-09T10:19:21Z

src/api/fetch-api.js

@@ -7,7 +7,7 @@ export const fetchAPI = async (url, options) => {
  const response = await fetch(`${config.apiUrl}/${url}`, {
    method: 'GET',
    headers: {
-      'Content-Type': 'application/json',
+      'Content-Type': 'application/vnd.api+json',


@prichodko If we don't use this Content-Type while creating customer it breaks the API :) I think we can use it everywhere.

apedroferreira · 2019-04-09T10:20:05Z

src/components/Input/index.js

+      <Label htmlFor={name}>{label}</Label>
+      <Field
+        name={name}
+        render={({ field }) => (


Had to use render here or the hasError prop throws a console error.

prichodko

Left a few inline comments, to simplify code even more :)

prichodko · 2019-04-09T11:30:34Z

src/components/Input/index.js

+
+import { Wrapper, Label, StyledInput, StyledError } from './styled'
+
+const Input = ({ error, isTouched, label, name, type = 'text' }) => {


You can tak error and touched from form props by the name and therefore you won't have to pass it in the actual form and leverage the Formik context.

return ( <Field name={name}> {({ field, form }) => { const error = form.errors[name] const isTouched = form.touched[name] const isInvalid = isTouched && Boolean(error) // ... ...

Thanks, didn't know this :)

prichodko · 2019-04-09T11:31:29Z

src/pages/SignUp/index.js

+              )}
+              <Input
+                name="firstName"
+                type="text"


You have declared type = 'text' as a default prop, so you don't have to pass it here.

prichodko · 2019-04-09T11:31:54Z

src/pages/SignUp/index.js

+                name="firstName"
+                type="text"
+                label="First name"
+                isTouched={touched.firstName}


You can get rid of isTouched and error prop. :)

* Add create-customer API call * Implement sign up form with styles and libraries * General layout improvement * ESLint and stylelint configuration fixes

* Code review fixes * Integration fixes * Use rem and themes in styles

* Formik Input refactoring * Omit default props

apedroferreira requested review from dannytce and prichodko April 7, 2019 12:23

apedroferreira force-pushed the week4 branch from b2daee1 to 1411893 Compare April 7, 2019 12:34

prichodko force-pushed the week4 branch from 71c5acf to 813c911 Compare April 8, 2019 09:21

dannytce reviewed Apr 8, 2019

View reviewed changes

apedroferreira force-pushed the week4 branch 2 times, most recently from 5839d56 to 5748815 Compare April 9, 2019 10:16

apedroferreira commented Apr 9, 2019

View reviewed changes

apedroferreira requested a review from dannytce April 9, 2019 10:34

prichodko suggested changes Apr 9, 2019

View reviewed changes

apedroferreira requested a review from prichodko April 9, 2019 14:02

koss-lebedev and others added 14 commits April 10, 2019 10:52

Week 3: Homework

47549b8

Week 4: Forms

e4f9a1b

* Add create-customer API call * Implement sign up form with styles and libraries * General layout improvement * ESLint and stylelint configuration fixes

Add token storage util

885d1bb

Refactor API client

c261b54

Add PrivateRoute component

7377ec4

Week 4: Forms - Fixes

ebf3dfe

* Code review fixes * Integration fixes * Use rem and themes in styles

Week 4: Forms - More fixes

e333102

* Formik Input refactoring * Omit default props

Rename fetchAPI to API client, fix paths

923a02f

Secure Account page instead of Cart

2f5118e

Rename get-access-token to get-guest-token

7da50b3

Refactor token getter in api client

c04c173

Add call to get customer token

cb1b229

Redirect after successful sign up

1b80d0c

Change session storage to localStorage

8caeea3

dannytce force-pushed the week4 branch from 1be6c9e to 8caeea3 Compare April 10, 2019 08:59

dannytce added 2 commits April 10, 2019 11:10

Resolve merge conflict 🤦‍♂️

2c241db

Enforce named imports

e60d8a6

dannytce added 4 commits April 10, 2019 11:39

Fix API token logic

14d19b2

Restructure products api

1abe585

Add lectures docs

e017e97

Move format-product util function

be074dc

dannytce merged commit ff97516 into master Apr 10, 2019

dannytce deleted the week4 branch April 10, 2019 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Week 4 #6

Week 4 #6

apedroferreira commented Apr 7, 2019

dannytce left a comment

dannytce Apr 8, 2019

dannytce Apr 8, 2019

dannytce Apr 8, 2019

apedroferreira Apr 8, 2019

dannytce Apr 8, 2019

dannytce Apr 8, 2019

apedroferreira Apr 8, 2019

dannytce Apr 10, 2019

dannytce Apr 8, 2019

dannytce Apr 8, 2019

apedroferreira Apr 8, 2019

dannytce Apr 8, 2019

dannytce Apr 8, 2019

apedroferreira commented Apr 9, 2019

apedroferreira Apr 9, 2019

apedroferreira Apr 9, 2019

prichodko left a comment

prichodko Apr 9, 2019

apedroferreira Apr 9, 2019

prichodko Apr 9, 2019

prichodko Apr 9, 2019


		import { Wrapper, Label, StyledInput, StyledError } from './styled'

		const Input = ({ error, isTouched, label, name, type = 'text' }) => {

Week 4 #6

Week 4 #6

Conversation

apedroferreira commented Apr 7, 2019

dannytce left a 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

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

apedroferreira commented Apr 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prichodko left a 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