PR for Video Library [FE] #1

purvasheth · 2021-05-05T18:39:04Z

No description provided.

Janaki2399 · 2021-05-08T18:05:22Z

Hey Purva, I totally loved your app. The code is very clean too.
I have few suggestions with the folder structure.
-You can create a separate folder for context
-In few pages, you have listed all the components in the same file for example video listing page. Instead, you can extract each of those components into a separate file. In the components folder you can create a separate folder like video, can group all the components of the videos page into that folder. This way the page would have only the main structure.
-You can also rename your JSX instead of js. It would be easier to distinguish between both.

Also for the functions, in few places, the arrow function is used and in few places, regular function syntax is used. You can change all to arrow functions so that your app will only have Es6 syntax and it will be consistent.

Janaki2399 · 2021-05-08T18:06:25Z

src/pages/Playlists/Playlists.js

+        const { _id, name, videos, defaultPlaylist } = playlist;
+        return (
+          <Fragment key={_id}>
+            <PlaylistHeader


You can create a playlist card component and extract this out. The main page structure would look more cleaner.

A playlist is a group of videos and does not have a card as such. So, I made the code readable by breaking it into more components. Thanks for noticing this.

Janaki2399 · 2021-05-08T18:17:41Z

src/pages/Videos/VideoDetail.js

+        navigate("/page-not-found");
+      }
+    })();
+    return () => {


It's nice that you have cleaned up the useEffect.I should do it too

Janaki2399 · 2021-05-08T18:23:38Z

src/pages/Videos/VideoDetail.js

+  useEffect(() => {
+    if (mongooseVideoId) {
+      if (history.length === 0) {
+        (async () => {


I feel it is getting a bit verbose here. Try extracting it to a separate function.

Good Idea. Created multiple functions for this

Janaki2399 · 2021-05-08T18:28:12Z

src/pages/Videos/VideoListing.js

+export function ToggleIconGroup({ video, likedVideos, watchLater }) {
+  return (
+    <>
+      <ToggleIcon playlist={likedVideos || defaultPlaylist} video={video} />


This || thing is so cool. I will use it in mine also. I used to pass an additional prop for this

Janaki2399 · 2021-05-08T18:30:51Z

src/custom hooks/useToggleVideo.js

+      }
+    } else {
+      const success = await removeVideoFromPlaylist(video._id);
+      if (success) {


You can extract these also to a separate function. It would be easier to understand.

Again a great suggestion. Updated this

purvasheth · 2021-05-08T19:00:12Z

Hey Purva, I totally loved your app. The code is very clean too.
I have few suggestions with the folder structure.
-You can create a separate folder for context
-In few pages, you have listed all the components in the same file for example video listing page. Instead, you can extract each of those components into a separate file. In the components folder you can create a separate folder like video, can group all the components of the videos page into that folder. This way the page would have only the main structure.
-You can also rename your JSX instead of js. It would be easier to distinguish between both.

Also for the functions, in few places, the arrow function is used and in few places, regular function syntax is used. You can change all to arrow functions so that your app will only have Es6 syntax and it will be consistent.

Thanks a lot for your Review. All your suggestions are great!
I'll make these changes.

For the folder structure, I have tried to group by functionality i.e all files related to 'Videos' are together. I don't know which is better, group by functionality or group by type.
Good catch on the video listing page having more components. I forgot to move components to different files.
As for the inconsistency in using arrow functions, I have used arrow functions for methods inside components and normal functions for components. My reason for using normal functions for components is because I try to follow the Airbnb react guide. https://github.com/airbnb/javascript/tree/master/react#class-vs-reactcreateclass-vs-stateless

Jaynil1611

Great work @purvasheth, Very nice and consistent UI
The code is very well maintained and the naming of the components is also perfect.

Here are my few suggestions

You can use index.js at the root folder wherever you feel it is required while using nested imports.
Use ternary operator where you are using if-else for small code
Just a suggestion, single file going over 100 lines, you can extract the components to separate files and import them

Jaynil1611 · 2021-05-09T11:17:10Z

src/App.jsx

+import { VideoListing } from "./pages/Videos/VideoListing";
+import { Playlists } from "./pages/Playlists/Playlists";
+import { PlaylistDetail } from "./pages/Playlists/PlaylistDetail";
+import { Routes, Route } from "react-router-dom";
+import { NavigationBar } from "./components/NavigationBar";
+import { VideoDetail } from "./pages/Videos/VideoDetail";
+import { PageNotFound } from "./pages/PageNotFound";
+import { History } from "./pages/History/History";
+import { SearchResults } from "./pages/SearchResults";
+import { ToastContainer } from "react-toastify";


Instead of doing separate imports for the same folder, you can use index.js inside page folder and re-export all nested components inside pages folder
e.g.

Suggested change

import { VideoListing } from "./pages/Videos/VideoListing";

import { Playlists } from "./pages/Playlists/Playlists";

import { PlaylistDetail } from "./pages/Playlists/PlaylistDetail";

import { Routes, Route } from "react-router-dom";

import { NavigationBar } from "./components/NavigationBar";

import { VideoDetail } from "./pages/Videos/VideoDetail";

import { PageNotFound } from "./pages/PageNotFound";

import { History } from "./pages/History/History";

import { SearchResults } from "./pages/SearchResults";

import { ToastContainer } from "react-toastify";

import { VideoListing, Playlists, PlaylistDetail,... } from "./pages";

Jaynil1611 · 2021-05-09T11:17:42Z

src/App.jsx

+      <Routes>
+        <Route path="/" element={<VideoListing />} />
+        <Route path="/playlists" element={<Playlists />} />
+        <Route path="/playlists/:playlistId" element={<PlaylistDetail />} />
+        <Route path="/video/:videoId" element={<VideoDetail />} />
+        <Route path="/history" element={<History />} />
+        <Route path="/search" element={<SearchResults />} />s
+        <Route path="*" element={<PageNotFound />} />
+      </Routes>


Extract this to a router.js file

Since the number of routes is less, I think it's ok to keep them in this file.

Jaynil1611 · 2021-05-09T11:22:21Z

src/components/NavigationBar.jsx

+import { NavLink } from "react-router-dom";
+import { usePlaylists } from "../pages/Playlists/playlists-context";
+import { SET_VIDEOS } from "../pages/Videos/videos-reducer";
+import { useAxios } from "../custom hooks/useAxios";


use consistent naming convention custom hooks rename to custom-hooks

Good catch. Will update this.

Jaynil1611 · 2021-05-09T11:29:16Z

src/components/NavigationBar.jsx

+      (!isLikedVideosIdSet || !isWatchLaterIdSet) &&
+      i < playlists.length
+    ) {


You could extract this logic out to function to improve readability

Jaynil1611 · 2021-05-09T11:40:35Z

src/components/PlaylistModal.jsx

+    </label>
+  );
+}
+function CreatePlaylist({


You could extract out CreatePlaylist and it's related components to new file

Jaynil1611 · 2021-05-09T11:51:20Z

src/pages/Playlists/PlaylistDetail.jsx

+        <h1>{name}</h1>
+        <div className="flex">
+          {videos.map((video) => {
+            return (


Again, here you can remove return same as before

Jaynil1611 · 2021-05-09T11:54:04Z

src/pages/Playlists/Playlists.jsx

+  );
+}
+
+function PlaylistHeader({ name, _id, defaultPlaylist }) {


Extract this component and below component to different file

Jaynil1611 · 2021-05-09T11:55:55Z

src/pages/Playlists/playlists-reducer.js

+        playlists: playlists.map((playlist) =>
+          playlist._id !== playlistId
+            ? playlist
+            : { ...playlist, videos: playlist.videos.concat(video) }
+        ),
+      };


Extract this and similar reducer actions to another function and use them,
And you can refactor it to this in ADD_VIDEO_ && REMOVE_VIDEO_

Suggested change

playlists: playlists.map((playlist) =>

playlist._id !== playlistId

? playlist

: { ...playlist, videos: playlist.videos.concat(video) }

),

};

playlists: playlists.map((playlist) =>

playlist._id === playlistId

? { ...playlist, videos: playlist.videos.concat(video) } : playlist

),

};

Jaynil1611 · 2021-05-09T12:16:40Z

src/components/BaseCard.jsx

+
+  return (
+    <div key={id} className="card card--shadow m-1">
+      <Thumbnail onClick={openVideo} id={id} className="clickable" />


The onClick={openVideo} function should be on the upper div and not on Thumbnail, due to which clicking on an avatar or video title, the video is not opening up.

Jaynil1611 · 2021-05-09T12:19:05Z

src/pages/Videos/VideoListing.jsx

+  const [activeCategory, setActiveCategory] = useState("All");
+  return (
+    <div className="container">
+      <h1>Video Listing</h1>


Instead of VideoListing, you can rename it to Explore Videos or anything else

purvasheth · 2021-05-11T21:56:20Z

Great work @purvasheth, Very nice and consistent UI

The code is very well maintained and the naming of the components is also perfect.

Here are my few suggestions

You can use index.js at the root folder wherever you feel it is required while using nested imports.

Use ternary operator where you are using if-else for small code

Just a suggestion, single file going over 100 lines, you can extract the components to separate files and import them

All your suggestions were great!
Tried to implement them as much as possible.
Thanks a lot for your review

purvasheth added 17 commits April 2, 2021 21:22

add custom playlists, watch later and like video feature

95f6150

refactor all code

a335c92

add routing using react-router-v6

7fb726d

remove commented router code

51588c6

add video detail page

eb8596a

add history feature

8e09213

add notes that support markdown

9ec8c82

changes notes styling

5537355

add category wise video listing

06768c6

404 route and minor fixes

529b22a

connect home page and playlist page to backend

f30b905

refactor single playlist page

15da314

add history feature with BE

94133ea

add search by name functionality

bcbb363

add toasts for feedback

a8c93ec

fix bugs and connect to repl

840200d

refactor files into folders and fix minor bugs

913f3ae

purvasheth changed the title ~~PR for Review of Video Library [FE]~~ PR for Video Library [FE] May 5, 2021

fix post search navigate

77a1b8b

Janaki2399 suggested changes May 8, 2021

View reviewed changes

handle review comments

ba22d99

Jaynil1611 suggested changes May 9, 2021

View reviewed changes

PR for Video Library [FE] #1

Are you sure you want to change the base?

PR for Video Library [FE] #1

Uh oh!

Conversation

purvasheth commented May 5, 2021

Uh oh!

Janaki2399 commented May 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purvasheth commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jaynil1611 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purvasheth commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

purvasheth commented May 8, 2021 •

edited

Loading

Jaynil1611 left a comment •

edited

Loading

purvasheth commented May 11, 2021 •

edited

Loading