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

Add files via upload #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

spidervamsi
Copy link

These 3 files have been changed for " I'm feeling lucky button in chrome extension"

These 3 files have been changed for " I'm feeling lucky button in chrome extension"
@abhay-raizada
Copy link
Member

@PaliwalSparsh is travelling maybe @djmgit should have a look at it if he's available? @spidervamsi It might take some time to review :P

@spidervamsi
Copy link
Author

I'll wait for him.Thank you so much @abhsag24 :)

Copy link
Member

@djmgit djmgit left a comment

Choose a reason for hiding this comment

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

@spidervamsi Thanks for the PR. Please see the comments below:
Please remove the two extra empty lines which you have added in popup.css.
Please remove the whitespace you have added before 'var index' in popup.js. Do try to maintain a consistent indentation.
You have referred a file named indexStuff.js in popup.html which is not present in the repo.
If i am not wrong your PR deals with adding I am feeling lucky button, however I could not relate the PR title with the change you have made. Moreover issue #35 deals with the same task. So it would be nice if you refer that issue in your PR. You can checkout CONTRIBUTING.md to see how to refer a issue

Thanks for the PR :)

@PaliwalSparsh
Copy link
Collaborator

@djmgit thanks for reviewing.

@spidervamsi Thanks for the PR. As @djmgit specified this is issue #35 . The bug had needs design label so we could have had a discussion before working on the PR. But now you have sent the PR so no problem with that. Just develop a habit to comment on PR before working on it.

As i can see from the changes you made, I'm feeling lucky button here directs us to the google doodle pages. That's great you added the button that is first part done right, just make sure to add the changes as specified by djmgit.

Next and the most important thing you would want to do is search for a way through which we can create a URL that lands us on the first result of the google results query (ie actual work that I'm feeling lucky does). Once you find the URL that would be used to perform this work, add the search query to it and then add this whole to the button. That would do the work.

I hope you would be able to make it work. Again thanks for the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants