Skip to content

Conversation

@SrushtiHaryan
Copy link
Collaborator

πŸ› οΈ Fixes Issue

πŸ‘¨β€πŸ’» Changes proposed

βœ”οΈ Check List (Check all the applicable boxes)

  • My code follows the coding standards of this project.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

πŸ“„ Note to reviewers

πŸ“· Screenshots

  • Type-Grid

Type-Grid

  • Trending Shorts

Trend

  • Responsive

Responsive

@SrushtiHaryan SrushtiHaryan linked an issue Jun 25, 2022 that may be closed by this pull request
4 tasks
Copy link
Contributor

@apoorv-on-git apoorv-on-git left a comment

Choose a reason for hiding this comment

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

Excellent work overall the result looks fantastic! A few things I would also mention in my review, but do note -

  1. The section mentioned contains categories. Now if you see, instead of the icons and the text, the design philosophy is the same. It might be better to render this from JS, just as you've done with the videos, using a list of objects containing icon names and text.
  2. Get rid of all the comments. It's not a good idea to raise pull requests with commented code. Comments should only be for essential bits of information describing the code or its functionality.
  3. Why don't you use the CDNs available for the owl carousel given here - https://cdnjs.com/libraries/OwlCarousel2 - You can take the minified links and add them in the headers of the explore.html file.
  4. Remove the screenshot directory. There's no need for that.

@unnikrishnan2002 unnikrishnan2002 changed the title Issue 7 feat: βœ…[Issue 7] Explore page Jun 26, 2022
Copy link
Contributor

@unnikrishnan2002 unnikrishnan2002 left a comment

Choose a reason for hiding this comment

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

I would request a couple of changes -

  • Don't edit the PULL_REQUEST_TEMPLATE.md file. You just have to edit the template which will appear in you PR comment while making a PR and not edit the file itself.
  • Maintain the formatting of CSS code. Keep a 4 space gap when you write properties, put the properties in alphabetical order, end the closing bracket in a new line, keep 1 space after each property block
  • Maintain the formatting of js file as well. currently its not uniform for the whole file. Keep a space of 4 whenever you have to nest elements inside the block

__
sema-logo Β Summary: πŸ› οΈ This code needs a fix

Copy link
Contributor

@unnikrishnan2002 unnikrishnan2002 left a comment

Choose a reason for hiding this comment

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

Remove the commented part from the explore.css file

removed commented part
Copy link
Contributor

@unnikrishnan2002 unnikrishnan2002 left a comment

Choose a reason for hiding this comment

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

LGTM

__
sema-logo Β Summary: πŸ‘Œ This code looks good

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.

Explore Page

4 participants