-
Notifications
You must be signed in to change notification settings - Fork 2
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
Open for Reviews! #1
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well-written code. I loved how you've made it modular by writing small components for everything and writing reusable functions separately.
import { Routes, Route } from 'react-router-dom'; | ||
|
||
import './App.css'; | ||
import NavBar from './Components/NavBar/NavBar'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use index.js
file in every folder to export everything so that all the components belonging to same type can be imported in a single line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the advice. Will surely implement this.
<h1 className="section-title">{title}</h1> | ||
<div className="grid-box"> | ||
{ | ||
[0, 1, 2, 3].map(item => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this section do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got left unchecked, during code refactoring. Will Delete this.
</NavLink> | ||
<div className="right-nav"> | ||
<ul> | ||
<NavLink to="/" className="mobile-hide">Home</NavLink> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These <NavLink>
s should be wrapped with <li>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Will fix this.
src/ReusableFunctions/funcs.jsx
Outdated
@@ -0,0 +1,35 @@ | |||
export function searchLikes(state, course) { | |||
if (state.likedVideo.filter((item) => item.id === course.id).length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can directly return the condition inside the if
block, everything else is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will refactor this.
src/Reducer/Auth-Reducer.js
Outdated
export function reducerFunc(state, { type, payload }) { | ||
switch (type) { | ||
case SAVE_SIGNUP_DETAILS: | ||
console.log("I ran") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this kind of code while publishing
src/Reducer/Video-Reducer.js
Outdated
export function dispatchFunc(state , { type, payload }) { | ||
switch (type) { | ||
case ADD_TO_LIKES: | ||
return { ...state, likedVideo: [...state.likedVideo, payload] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use an object while sending payload
so that all the contents of the payload are easily understood here in reducer
src/Reducer/Video-Reducer.js
Outdated
return {...state, myNotes : [...state.myNotes, {id : payload.id, notes : [payload.input]}]} | ||
} | ||
|
||
case ADD_NEW_PLAYLIST: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you send payload
as { playlistName: 'XYZ' }
so that here you can assign as `{ name: payload.playlistName } which is more meaningful and understandable
src/Reducer/Video-Reducer.js
Outdated
searchValue : "" | ||
} | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this commented code
src/ReusableFunctions/funcs.jsx
Outdated
} | ||
|
||
export function returnUserID(state, user) { | ||
return state.userLoginDetails.find(one => one.email === user.email && one.password === user.password).userId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, this is how you should write all other functions in this file!
Please Review, Sparrow Visuals (AKA Dove), and suggest feedbacks and changes to improve the Video Library.
For a better view of the Code -
Here's a live link to the codesandbox or Online VS Code
Some Key Points to help you , though the code maze,
src
folder contains all the files, related to React.pages
folder contains different pages/ screens, which constitutes the Video Library.Components
Folder.