Skip to content

Challenge 5#3

Open
erfemega wants to merge 3 commits into
masterfrom
Challenge_5
Open

Challenge 5#3
erfemega wants to merge 3 commits into
masterfrom
Challenge_5

Conversation

@erfemega

@erfemega erfemega commented Apr 1, 2021

Copy link
Copy Markdown
Owner

This PR contains:

  • Authentication api mocked
  • Add and remove favorites button in video detail
  • Favorites and FavoriteDetail views are protected
  • Favorites menu button is not visible if user is not authenticated
  • Auth session and favorites are stored in LocalStorage
  • Components rendering tests

@erickwize erickwize left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comments

Good work. I left some recommendations.
I see that your tests are not expecting anything, you're only rendering the component, I encourage you to add expect conditions for testing correctly the application and get the certificate.

Acceptance Criteria

  • All the sections have their own route.
  • [] Navigation across the sections is implemented correctly.
  • The Global State is persistent across all the sections.
  • The "Mocked Authentication" mechanism works correctly.
  • The "session data" is stored in the Global Context correctly.
  • Videos can be added to the Favorites list.
  • Videos can be removed from the Favorites list.
  • Navigation to Favorite Videos View using a private route is implemented correctly (only authenticated users should access this section).
  • Navigation to the Favorite Video Details View using a private route is implemented correctly (only authenticated users should access this section).
  • Information for the selected favorite video is displayed correctly
  • The list of other favorite videos in the Favorite Video Details View is displayed.
  • Testing coverage is above 70%. (Please include a screenshot of the code coverage report in your PR description).

Bonus Points

  • The Add/remove from favorites button should appear when the user passes the mouse over the video card in the list.
  • Integrate with a real authentication provider (such as Auth0, OAuth or Firebase).
  • A 404 section is shown when a route is not found.
  • The Login View is implemented as a modal using React Portals.

Comment thread src/api/auth.js Outdated
Comment on lines +45 to +55
<ProtectedRoute
path="/favorites"
isAuthenticated={globalState.authenticated}
Component={Favorites}
/>
<DetailContextProvider>
<ProtectedRoute
path="/favoriteDetail"
isAuthenticated={globalState.authenticated}
Component={FavoriteDetail}
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

With the current routing implementation, the user won't be able to share links, because the routing depends on the storage. To handle correctly routing you should use parameters in the path: /favorites/:id and use useParams to obtain the id.
i.e.

import { useParams } from 'react-router-dom';

function Component() {
  const { id } = useParams();
}

Comment thread src/components/FavoriteButton/FavoriteButton.jsx Outdated
</ToggleWrapper>
</ButtonWrapper>
);
return profileButton;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I recommend returning the JSX here, it is not adding more readiness with the name of the variable

function RelatedVideosList({ videos }) {
console.log('----', videos);
function RelatedVideosList({ videos, title }) {
let related = videos

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why did you use let? If the value is not going to change, it is recommended to use const so we keep close to the functions without side effects

<Wrapper>
<Title>Try a Video</Title>
<VideosList videos={globalState.videos} />
<VideosList videos={globalState.videos} itemPath="/detail" />

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I suggest adding the id to the path /detail/<video_id>

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.

2 participants