Skip to content

Conversation

Vishal-raj-1
Copy link
Contributor

@Vishal-raj-1 Vishal-raj-1 commented Jan 5, 2021

Description of changes made

Submissions guide:

  • Have you made corresponding changes to the documentation?
  • Your submission doesn't break any existing feature.
  • Have you lint your code locally prior to submission?

@s-ayush2903 @Ankit7Das Please review it before NWOC ends !!

P.S: I have learn a lot while doing this !! 🔥

@vercel
Copy link

vercel bot commented Jan 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/s-ayush2903/nwoc-website-alpha-version/92m9sy3z0
✅ Preview: https://nwoc-website-alpha-version-git-fork-vishal-raj-1-project.s-ayush2903.vercel.app

Copy link
Member

@s-ayush2903 s-ayush2903 left a comment

Choose a reason for hiding this comment

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

First of all, revert all the indentation changes and un-necessary whitespace/newline additions in the files. I can see you applied your local indentation on the projects which has resulted in sheer size of the PR, please revert changes like this to speed up the review process because even if we merge this(with un-necessary indentation changes), then other PRs will result in conflicts and then everyone will have to resolve it own their own, which in itself can prove to be a time consuming process

@Vishal-raj-1
Copy link
Contributor Author

Ok 👌 Will send it you tomorrow 😊

@Vishal-raj-1
Copy link
Contributor Author

@s-ayush2903 @Ankit7Das Please review it !! 🔥

@s-ayush2903 s-ayush2903 requested a review from Ankit7Das January 6, 2021 13:11
@s-ayush2903
Copy link
Member

s-ayush2903 commented Jan 6, 2021

@Vishal-raj-1 I can see you've messed up your commit history, I don't want to press this point to clean the history, but you should revert the changes in the file that ain't related to the subject of the issue which this PR targets, like for example, for this PR there should be no changes visible in contact.html & archive.html as this PR aims to implement search feature for Projects section only, and also the changes made their are just order & indentation changes which is resulting in the sheer size of this PR, you should revert 'em, to be specific, for this PR changes should be visible in only style.css and projects.html files, I'm pretty sure this can be done quite easily. This problem is with all your current PRs, you should now revert the changes in files that ain't related to the issue subject(a time saving hack for it is to copy and paste the unrelated files from the mainstream and then push) and it should work fine

Also you can maintain clean commit history by keeping your master/dev branch clean and regularly up to date with the main repo's active branch whenever you create a new commit/PR, create a new branch from your clean branch, this'll ease stuff for you and reviewer as well.

@Vishal-raj-1
Copy link
Contributor Author

let me do one thing, I am closing other two PR and solving three issues in one PR !!

@Vishal-raj-1
Copy link
Contributor Author

Vishal-raj-1 commented Jan 6, 2021

@Vishal-raj-1 I can see you've messed up your commit history, I don't want to press this point to clean the history, but you should revert the changes in the file that ain't related to the subject of the issue which this PR targets, like for example, for this PR there should be no changes visible in contact.html & archive.html as this PR aims to implement search feature for Projects section only, and also the changes made their are just order & indentation changes which is resulting in the sheer size of this PR, you should revert 'em, to be specific, for this PR changes should be visible in only style.css and projects.html files, I'm pretty sure this can be done quite easily. This problem is with all your current PRs, you should now revert the changes in files that ain't related to the issue subject(a time saving hack for it is to copy and paste the unrelated files from the mainstream and then push) and it should work fine

Description of changes made

@s-ayush2903 @Ankit7Das Please review it before NWOC ends !!

@Vishal-raj-1 Vishal-raj-1 changed the title adding search bar for project page adding search bar for project page, adding icons on contact page and dropdown list on archive page. Jan 6, 2021
@Vishal-raj-1 Vishal-raj-1 changed the title adding search bar for project page, adding icons on contact page and dropdown list on archive page. adding search bar for project page, and dropdown list on archive page. Jan 8, 2021
@Vishal-raj-1
Copy link
Contributor Author

@s-ayush2903 @Ankit7Das Any update ? Now This PR solve two issues #99 and #91 . For these issues we have to change in archive.html , project.html and style.css .

I have update PR message and title for the same !!

@s-ayush2903
Copy link
Member

@Vishal-raj-1 This isn't how it works, I've told you multiple times that your PR unnecessarily large due formatting changes and I can still see 'em present here, in order to get it merged you'll have to fix this problem

@Vishal-raj-1
Copy link
Contributor Author

@Vishal-raj-1 This isn't how it works, I've told you multiple times that your PR unnecessarily large due formatting changes and I can still see 'em present here, in order to get it merged you'll have to fix this problem

oh formatting changes. I am doing it right away.

@Vishal-raj-1
Copy link
Contributor Author

@s-ayush2903 @Ankit7Das Yep !! It's done now. I have understand my mistake. I have copied archive.html and project.html from master then copy the changes from this PR !! and it's work. Now it is acceptable.

@Vishal-raj-1
Copy link
Contributor Author

for #103 . I am doing changes in this PR only as it is beginner level problem doesn't need to more PR for this. It fixes #103

@s-ayush2903
Copy link
Member

Good job @Vishal-raj-1! Nice work

Copy link
Member

@s-ayush2903 s-ayush2903 left a comment

Choose a reason for hiding this comment

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

LGTM

@s-ayush2903 s-ayush2903 merged commit 486fef4 into NJACKWinterOfCode:master Jan 9, 2021
@Vishal-raj-1
Copy link
Contributor Author

Good job @Vishal-raj-1! Nice work

Thanks Sir !!

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.

We Should remove student registration popup message. Archive Page using summary and details tag add search bar for selection of projects
2 participants