Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added I'm feeling lucky button and made the display of google search result more meaningful #69

Closed
wants to merge 4 commits into from

Conversation

iamrishab07
Copy link
Contributor

Add I'm feeling lucky button at the footer.
Fix #35

Modify googleCS.js to make the titles of the google search results more meaningful by removing the leading "Index of " part from the title.

Fix #45

@abhay-raizada
Copy link
Member

@iamrishab07 you seem to have errors on your PR see https://travis-ci.org/NIT-dgp/chrome-search-extension/builds/194849346 for more details

@abhay-raizada
Copy link
Member

also it's always nice to show screenshots of work in action whenever making a PR of such sorts :D

@iamrishab07
Copy link
Contributor Author

Here's the code. There are some errors in the code regarding proper spacing , indentation and variable declaration.

screenshot 21

@abhsag24 should I directly commit again after correcting these errors or set another pull request closing the current one ?

@abhay-raizada
Copy link
Member

abhay-raizada commented Jan 24, 2017

@iamrishab07 i meant screenshots of "Fearch" working with the new changes you added :P and you can do git commit --amend to edit the last commit or git rebase -i HEAD^^^ to edit the last 3 commits (each hat means 1 commit back). Also for further chats please ping us at our gitter channel: https://gitter.im/NIT-dgp/General

@iamrishab07
Copy link
Contributor Author

Here's a screenshot of " Fearch " in action after the changes

screenshot 24

@abhay-raizada
Copy link
Member

@iamrishab07 nice @PaliwalSparsh will get back to you shortly :)

@PaliwalSparsh
Copy link
Collaborator

PaliwalSparsh commented Jan 24, 2017

@iamrishab07 Thanks for the PR 😄

Firstly it would be great if you try to have different PRs for different issues. I hope you will understand the reason for it by the end of this comment.

Actually both #35 and #45 were needs design, so they require a bit of discussion. Next time try to first comment on an issue and then work on it. It will be easier if the maintainers give you some clue of their intentions so later you need not make much changes to your work.

#35 was just not about adding the button, the google's url query for the I'm Lucky button was something that I searched very much but was not able to find. I expected that someone who approaches this issue, figures how to do this and makes the button work properly.

#45 is where this PR is going really great. I would really appreciate if you cherry-pick the #45 part from this PR and send in a new separate PR. We will make some more small changes to it and merge it.

Moral - Had you done both the issues on different branches from the beginning you would have not to do the extra work.

#35 is also a good issue to work on - you can also make a new separate PR for it and we shall have discussion on it too.

Again thanks for your work 😄

function lucky(){
var lButton = document.getElementById("imfl");
lButton.addEventListener("click", function () {
window.open("https://www.google.com/doodles");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everytime the window that gets open is that of doodles page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah , whenever the search query is empty ,this code will work for that only.
@PaliwalSparsh I'm working on it .

Copy link
Collaborator

Choose a reason for hiding this comment

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

@iamrishab07 Great, do let me know if you have any problem.

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