Recipe project#2
Conversation
…st correctly for the TOKEN
| @@ -0,0 +1,44 @@ | |||
| /*.App {*/ | |||
There was a problem hiding this comment.
In our projects we would block a PR leaving commented code 😄
| jest.mock("axios"); | ||
|
|
||
| afterEach(() => { | ||
| cleanup() |
There was a problem hiding this comment.
You don't need to do this anymore, testing-library automatically invokes cleanup after each test now
| @@ -0,0 +1,134 @@ | |||
| import React from "react"; | |||
| import {cleanup, fireEvent, render} from "@testing-library/react"; | |||
There was a problem hiding this comment.
You could add waitForElement to the imports from this file (and remove it from @testing-library/dom)
| expect(container.querySelector("input[name=email]")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // test("an user can logout", () => { |
There was a problem hiding this comment.
Don't leave commented code, not even for tests 😉
| margin-top: 50px; | ||
| `; | ||
|
|
||
| const Image = styled.img.attrs(props => ({ |
There was a problem hiding this comment.
You don't need the .attrs(props => ({ ...props }) part, it can just be
styled.img`max-width: 70;`
| addingIngredient | ||
| ? <form onSubmit={handleNewIngredientCreation}> | ||
| <Input type="text" id="ingredientInput" value={newIngredient} onChange={handleNewIngredientChange}/> | ||
| <Button primary onClick={handleNewIngredientCreation}><span>Create</span></Button> |
There was a problem hiding this comment.
You probably wouldn't need that inner span node
| return ( | ||
| <Route | ||
| render={() => | ||
| token !== null |
There was a problem hiding this comment.
Probably token !== null is a bit too strict, could just be token ? <>{/* private */}</> : <>{/* public */}</>
| import RecipeDetails from "./RecipeDetails"; | ||
| import {AuthContext} from "./contexts/AuthenticationProvider"; | ||
|
|
||
| export function PrivateRoutes() { |
There was a problem hiding this comment.
The file is called PrivateRoutes but it actually contains the public too, why not calling it just Routes?
| } | ||
|
|
||
| async function modifyUser(password, name) { | ||
| if ((name === "" || name === undefined) && (password === "" || password === undefined)) |
There was a problem hiding this comment.
(name === "" || name === undefined) => (!name) (same for password)
| } | ||
|
|
||
| async function fetchTags() { | ||
| console.log("Fetching tags for: [" + token + "]"); |
There was a problem hiding this comment.
FYI in our web apps it won't be possible to ship code calling console.log (I've seen it being used quite a lot in this project)
| return ( | ||
| <option key={i} value={item.id}>{item.name}</option> | ||
| ) | ||
| }, this)} |
There was a problem hiding this comment.
You don't need the this argument here, because:
- you're in a functional component and not a class, so its value would be pretty useless
- you're using an arrow function which wouldn't rebind
thishttps://www.freecodecamp.org/news/when-and-why-you-should-use-es6-arrow-functions-and-when-you-shouldnt-3d851d7f0b26/#main-benefit-no-binding-of-this-
| } | ||
| } | ||
|
|
||
| const renderRedirect = () => { |
There was a problem hiding this comment.
I wouldn't put this as a function, I'd just add the if inside the body of the functional component:
if (redirect) return <Redirect />
return (
<>
<Container />
</>
)
| background: white; | ||
| font-weight: 400; | ||
| border: 2px solid red; | ||
| -webkit-box-sizing: border-box; |
There was a problem hiding this comment.
styled-components will automatically apply autoprefixer to your styles, no need to prefix them on your own
giuband
left a comment
giuband
left a comment
There was a problem hiding this comment.
Left a bunch of comments on what wouldn't have been approved on web or any other FE project of ours, if you have any doubts or questions feel free to reach me out on slack
| return ( | ||
| <> | ||
| {renderRedirect()} | ||
| { |
There was a problem hiding this comment.
It's a bit uncommon to leave a redirect inside the body, I would just put a separate if before this return.
if (redirect) return <Redirect to="/app" />
return (
<>
<Container />
</>
2 participants
FE for Django API: