Development Complete #1

AnkitaBagale · 2021-04-11T21:23:17Z

No description provided.

supminn

Hi @AnkitaBagale
Commendable work on this project! It's great to see that the code is broken down into smaller components and re-used very aptly.
I could find a few consistency issues in the naming conventions.

supminn · 2021-04-14T13:32:17Z

src/database.js

@@ -0,0 +1,592 @@
+export const types = ["All","Pencil Sketching",  "Charcoal Painting", "Painting", "Drawing"]
+// export const videosByTypes = {


Kindly omit commented code.

supminn · 2021-04-14T13:33:43Z

src/database.js

+    level:"Beginner",
+    language: "English",
+    thumbnail:"https://i.postimg.cc/TwsBcV04/jess-bailey-l3-N9-Q27z-ULw-unsplash.jpg",
+    tutor: {id:1, name: "SchaeferArt", avatar: "https://i.postimg.cc/fRf3jmN7/post-icon.png"}


this structure looks good for now. Nevertheless, when the data increases - say you have more than 3 tutors for the same video, following this structure could be difficult.
Instead, you can make "tutor" and array of tutor ids. Then a separate database could be created for all the tutor details.

Yes, that's a great idea, I did not think of it. I will refactor it when doing the backend.

supminn · 2021-04-14T13:35:59Z

src/App.js

-    <div className="App">
-      <h1>U&I Video Library</h1>
-    </div>
+    <Router>


Is there a specific reason to mention Routers here instead of using it on index.js?

Oh! thanks for pointing it, I will move it to index.js file

supminn · 2021-04-14T13:37:04Z

src/App.js

+
+              <Route path="/playlists" element={<AllPlaylistContent />} />
+              <Route path="/playlists/:playlistId" element={<PlaylistMainContent />} />            
+              <Route path="/saved" element={<WatchLater />} />


The path could be renamed to watchLater to keep it consistent

supminn · 2021-04-14T13:47:59Z

src/App.css

+.btn-text-icon-secondary .btn-icon.padding-0{
+  padding: 0;


this could be combined in line 113

supminn · 2021-04-15T06:23:31Z

src/Listing-pages/Watch-later.js

+            <div className="padding-left-1rem  padding-right-1rem">
+                <img className="img-responsive" src="https://i.postimg.cc/TwsBcV04/jess-bailey-l3-N9-Q27z-ULw-unsplash.jpg"  alt="watchLater" />
+                <div className="text-container">
+                    <h2 className="h4 padding-top-1rem">Saved Videos</h2>


When h4 is used within the className, the tag could also be replaced to h4 instead of h2 right?

Title of the page should be h1 and then sub heading of the page should be h2, for semantics, I am not sure, I will check this and let you know.

supminn · 2021-04-15T06:25:06Z

src/Listing-pages/explore.css

+
+.category-list{
+    display: grid;
+    grid-template-columns: 1fr 1fr 1fr 1fr;


could be simplified as: grid-template-columns: repeat(4, 1fr);

Yeah, i will use this.

supminn · 2021-04-15T06:29:42Z

src/Video-detail/notes-container.js

@@ -0,0 +1,133 @@
+import { getNoteDetails } from "../utils";
+import { v4 as uuidv4 } from 'uuid';
+


Contents of this file could be simplified by refactoring the components as new files. (since this file is named as notes-container, i was expecting only container details. However, this has all the implementation w.r.t notes)

Yeah i will create separate folder for notes and create files for each component related to notes

supminn · 2021-04-15T06:30:35Z

src/Video-detail/video-detail.css

@@ -0,0 +1,129 @@
+.video-container{
+    width: 100%;
+    height: 390px;


Avoid using px, instead rem is suggested to be used.

Done, updated all px values to rem

supminn · 2021-04-15T06:31:31Z

src/Video-detail/video-detail.css

+}
+.notes-section{
+    width: 30%;
+    height: 390px;


same here, to make it consistent with all the other properties, you could use rem instead of px.

payalkherajani · 2021-05-04T10:41:50Z

src/App.js

+
+    (async()=>{
+      try {
+        const { data: {response} } = await axios.get("https://uandistoreapi.herokuapp.com/videos");


Put URL of backend in some config file and then use it. Code looks more clean

payalkherajani · 2021-05-04T10:42:22Z

src/App.js

+        const { data: {response} } = await axios.get("https://uandistoreapi.herokuapp.com/videos");
+        dispatch({type: "SET_VIDEOS", payload: response})
+      } catch(error) {
+        console.log(error);


Remove console. You can use toast.error here

payalkherajani · 2021-05-04T10:49:28Z

src/App.js

+            <Route path="*" element={<ErrorPage />} />
+            <Route path="/error" element={<ErrorPage />} />


I am not sure of this but I think /error route is not required if element is rendered for all except those which are defined.

I navigate pages to error page if video URL is broken, hence need this route.

payalkherajani · 2021-05-04T10:53:06Z

src/Components/Authentication/ForgotPasswordPage.jsx

+                style={{
+                  display: fieldErrors.usernameError ? "block" : "none"
+                }}


Avoid inline styling

payalkherajani · 2021-05-04T10:57:23Z

src/Components/Authentication/ForgotPasswordPage.jsx

+            style={{
+              display: error !== "" ? "block" : "none",
+              color: "#ff9204"
+            }}


please remove inline styling

payalkherajani · 2021-05-04T11:16:27Z

src/Components/Authentication/Profile/ProfilePage.jsx

+            setUserDetails(response);
+          }
+        } catch (error) {
+          console.log(error);


Please remove console

payalkherajani · 2021-05-04T11:19:04Z

src/Components/Footer/Footer.jsx

+      <a href={toTop} className="btn btn-float-action" id="back-to-top">
+        <i className="fas fa-arrow-up"></i>
+      </a>


Avoid using tag, you can use Link tag here

This is vanilla js way to go to top. I will check if there is any other react way to go to top of the page.

payalkherajani · 2021-05-04T13:06:29Z

src/Components/Listing-pages/WatchLater.jsx

+                    <li key={video._id} className="badge-container" style={{width:"100%"}}>  
+                        <Link className="link-no-style" to={`/explore/${video._id}`}>


inline styling, please remove this

payalkherajani · 2021-05-04T13:10:41Z

src/Components/VideoDetailPage/AddToPlaylistContainer.jsx

+                <form onSubmit={(e)=>{
+                        e.preventDefault();
+                        createNewPlaylist({dispatch, userId, video, title:playlistTitle, setPlaylistTitle})
+                        setAddPlaylistInput(false);
+                        setAddtoPlaylist(flag => !flag);
+
+                    }}>


extract onSubmit has a separate function for code to look much cleaner

payalkherajani · 2021-05-04T13:13:29Z

src/Components/VideoDetailPage/Notes/NoteEditor.jsx

+                            if(noteState._id) {
+                                UpdateNoteForVideo({
+                                    noteId: noteState._id,
+                                    setNotes, 
+                                    noteUpdates: {   
+                                        title: noteState.title, 
+                                        description: noteState.description
+                                    },
+                                    setEditMode
+                                });
+                            } else {
+                                CreateNoteForVideo({
+                                    setNotes, 
+                                    newVideo: {   
+                                        userId, 
+                                        videoId, 
+                                        title: noteState.title, 
+                                        description: noteState.description,
+                                        time: playerRef.current.getCurrentTime()
+                                    },
+                                    noteDispatch
+                                });


extract this functionality into a function outside return please

payalkherajani · 2021-05-04T13:13:58Z

src/Components/VideoDetailPage/Notes/NoteEditor.jsx

+                            if(noteState._id){
+                                setEditMode(false);
+                            }else{
+                                noteDispatch({type:"SET_DESCRIPTION", payload:""});
+                                noteDispatch({type:"SET_TITLE", payload:""})
+                            }
+                        }} 


same here also

payalkherajani · 2021-05-04T13:14:44Z

src/Components/VideoDetailPage/Notes/NoteEditor.jsx

+                return {...state, description: payload}
+            }
+            default : {
+                console.log("this case is not handled with reducer")


remove console please

sakshijainn · 2021-06-14T17:45:30Z

the side bar watch later and history are not working
Also have some colorful content curation . All thumbnails are same.

sakshijainn · 2021-06-14T17:49:28Z

Very well structured code .
Just remove console in between the codes.

fix: small UI issues

fix: spell

Ankita fix nitpicks

fix: footer color

AnkitaBagale added 10 commits April 5, 2021 14:28

feat: router v6

7aac1b9

fix: warnings

34ccd1a

fix: warning

8ad9d1b

feat: category filter

104a47a

feat: add note

eef4208

fix warning

6fd4136

feat: sidebar

7bde13a

feat: homepage added

a3fbd91

fix: warnings

b5f2475

refactor: explore filter

e434470

supminn reviewed Apr 15, 2021

View reviewed changes

AnkitaBagale added 5 commits April 25, 2021 22:31

refactor: review comments

7a86268

refactor: files

462ddd1

feat: auth added

6f9d5c1

completed api calls

3df9929

fix: warnings

2d3634c

payalkherajani reviewed May 4, 2021

View reviewed changes

AnkitaBagale added 2 commits May 4, 2021 17:20

fix: video datail page error handling

7b1a533

fix: video id undefined

215ec4f

payalkherajani reviewed May 4, 2021

View reviewed changes

AnkitaBagale added 9 commits May 22, 2021 14:33

fix: sign up and forget password issue

cdb4c56

fix: data persistence after logout

196b6bb

feat: JWT authentication

7816a29

fix: api url

b9a0752

fix: 401 error handler

5d4507c

fix: forgot password api url

83a64e5

fix: status codes

c6e62a0

feat: share video

e4f6f93

fix: share url

3b49338

AnkitaBagale and others added 19 commits June 15, 2021 19:42

fix: change share url

3b8b33c

Update README.md

3e61b5e

fix: small UI issues

4995051

fix: nav items dry

031118c

Merge pull request #2 from AnkitaBagale/ankita-fix-nitpicks

1dbd257

fix: small UI issues

fix: spell

6f9e4f1

Merge pull request #3 from AnkitaBagale/ankita-fix-nitpicks

f4e40fc

fix: spell

fix: axios error handler

cd83829

fix: warning

80ff8bc

Merge pull request #4 from AnkitaBagale/ankita-fix-nitpicks

37222e5

Ankita fix nitpicks

fix: footer color

c2f3ece

Merge pull request #5 from AnkitaBagale/ankita-fix-nitpicks

b85d6b2

fix: footer color

feat: login with test creds

a24e1c4

fix: enter key press in search bar

62008b4

fix: image sizes

199b814

add: login on sign up

d136d5a

fix: api url

06e6081

fix: liked videos deletion

b2e9f65

fix: liked videos deletion

47999ac

		@@ -0,0 +1,592 @@
		export const types = ["All","Pencil Sketching", "Charcoal Painting", "Painting", "Drawing"]
		// export const videosByTypes = {

		@@ -0,0 +1,133 @@
		import { getNoteDetails } from "../utils";
		import { v4 as uuidv4 } from 'uuid';

		<Route path="*" element={<ErrorPage />} />
		<Route path="/error" element={<ErrorPage />} />

		<li key={video._id} className="badge-container" style={{width:"100%"}}>
		<Link className="link-no-style" to={`/explore/${video._id}`}>

Development Complete #1

Are you sure you want to change the base?

Development Complete #1

Conversation

AnkitaBagale commented Apr 11, 2021

supminn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnkitaBagale May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshijainn commented Jun 14, 2021

sakshijainn commented Jun 14, 2021

AnkitaBagale May 5, 2021 •

edited

Loading