Skip to content

Frontend completed#1

Open
rohan9896 wants to merge 34 commits intomainfrom
development
Open

Frontend completed#1
rohan9896 wants to merge 34 commits intomainfrom
development

Conversation

@rohan9896
Copy link
Owner

No description provided.

…eoCardExpand, VideoCardGrid, VideoSuggestionsCard, Context for liked/history/watchLater
…and fixed: warnings by adding key to every mapping
Copy link

@sandeepmehta2155 sandeepmehta2155 left a comment

Choose a reason for hiding this comment

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

Amazing UI and very clean View

src/App.js Outdated
@@ -1,11 +1,109 @@
import './App.css';
import { Routes, Route } from "react-router-dom";
import CategoryScroller from "./components/CategoryScroller/CategoryScroller";

Choose a reason for hiding this comment

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

In app.js import files from index.js to make code look clean

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done

<>
{state.watchLaterArr.length === 0 ? (
<NoVideosFoundPage />
) : (

Choose a reason for hiding this comment

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

Can use switch case but it's ok as logic is small

Copy link
Owner Author

Choose a reason for hiding this comment

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

As there are only two things, i think ternary operator is fine

},
{
topic: "DataScience",
id: uuid(),

Choose a reason for hiding this comment

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

When data is reloaded, this might get change & effect your data updation

Copy link
Owner Author

Choose a reason for hiding this comment

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

This id is only used for passing in the key prop of categories, it wouldn't make a diffrence

dispatch({type: "SEARCH_VIDEOS", payload: inputVal})
navigate(`/search?q=${inputVal}`);
}}>
<span role="img">🔍</span>

Choose a reason for hiding this comment

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

add a aria-labelled tag

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

Copy link

@sandeepmehta2155 sandeepmehta2155 left a comment

Choose a reason for hiding this comment

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

Things had been reviewed, wishing all the best ahead

@shubham-ghuge
Copy link

Hey Rohan,
The code and overall app looks good, I have reviewed the app here are some suggestions and fixes,

  1. Can change/remove default favicon.
  2. When i click directly on search button without adding any text the page goes blank.
  3. Give fixed position to the header and category submenu.
  4. when clicking on video the iframe size can be increased to maybe around 60vh or something.(Desktop view)
  5. Search button style can be improved.

@rohan9896
Copy link
Owner Author

  1. Can change/remove default favicon.
  • done
  1. When i click directly on search button without adding any text the page goes blank.
  • fixed
  1. Give fixed position to the header and category submenu.
  • gave fixed position to category, fixed header wasn't looking good so avoided that
  1. when clicking on video the iframe size can be increased to maybe around 60vh or something.(Desktop view)
  • fixed
  1. Search button style can be improved.
  • done

Copy link

@shubham-ghuge shubham-ghuge left a comment

Choose a reason for hiding this comment

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

Thank you for considering the changes Rohan,
I'm approving the PR.

rohan9896 added 2 commits May 26, 2021 03:12
Removed backend, will add that on separate branch so that i dont mess up my front end
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