Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Frontend/#062 user dashboard #115

Merged
merged 11 commits into from
Apr 6, 2021
Merged

Conversation

vieraboschkova
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@aleksanderwalczuk aleksanderwalczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Login

client/src/domain/Auth/Login/index.js
Move conditional jsx return from OnSubmit to separate const. Then you might finish this way:

return !isAuthenticated ? <NoAuth/>: <Redirect to=""/>

Navbar

client/src/components/Navbar/index.js
This is a mess, please clean it up.
Remember to use PascalCase on react components.

‼️ Don’t call components like functions

          {!isAuthenticated ? unauthenticatedNavBar() : authenticatedNavBar()}

✅ Use JSX instead

          {!isAuthenticated ? <NoAuthNavBar/> : <AuthNavBar/>}

Btw fix return logic above. If is authenticated return <AuthNavBar/> else return NoAuth

Signup

client/src/domain/Auth/SignUp/index.js
It would be good if you use a package to handle forms.
My proposals are React Hook Form or Formik.
Inputs have no labels

Remove resetForm function, it’s just code repetition

Same old story with auth, nonauth and ternary returning jsx

MovieSlider

client/src/domain/MovieSlider/index.js

If you combine jsx with array methods it’s common to make a single iteration component

‼️

       {posters.map((poster) => {
          return (
            <div key={poster} className="slider_container">
              <img className="slider_container_img" src={poster} alt="" />
            </div>
          );
        })}

       {posters.map((poster) => (<PosterItem key={/*lodash*/} poster={poster}/>))

Comment on lines 24 to 25
{posters.map((poster) => {
console.log(poster);
return (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this return anymore

isAuthenticated: true,
loading: false,
user: payload.user,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what else do you have in the payload?

Comment on lines +9 to +12
user: {
id: undefined,
isAdmin: undefined,
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why you define a user but all his\her props are undefined? 🤔

@@ -18,8 +18,8 @@ function Navbar() {
if (!isLoggedOut) {
alert('Could not log out user. Try again');
}
return <Redirect to="/movies" />;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you make a redirect on Navbar level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After logging out, whoever is at the page is redirected to the landing page.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but is he\she on a Navbar page? 🧐 What I mean is that navbar is not the best place to put authorization logic

Comment on lines 23 to 24
const unauthenticatedNavBar = () => {
return (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip the return

@@ -0,0 +1,33 @@
/* eslint-disable react/destructuring-assignment */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linter is right 😉

loadUser(dispatchUserContext);
return;
}
alert('Cannot delete order at this moment');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid using JS alert

Comment on lines +27 to +37
const userOrders = user.orders
? user.orders.map((order) => {
return (
<OrderItem
key={order._id}
id={order._id}
callback={handleOrderDelete}
/>
);
})
: undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set null instead of undefined. If component returns null it won't be rendered

}}
>
<h2>Orders</h2>
<ul>{userOrders !== undefined ? userOrders : <li>No orders</li>}</ul>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      <ul>
        {userOrders ? userOrders : <li>No orders</li>}
      </ul>

return (
<li className="profile__list-item">
<p>
{title}: <span className="profile__list-item--bold" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why span is self closing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why indeed :)

@vieraboschkova vieraboschkova marked this pull request as ready for review April 6, 2021 20:18
@vieraboschkova vieraboschkova merged commit 51c3943 into develop Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants