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

#124 Implement Search history #129

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

phoenix7139
Copy link

  • popup.js: Store the query searched by the user and corresponding URL to localStorage.
  • options.js: Retrieve stored query and URL from localStorage and display.
  • options.html, options.css: revamp the UI - add back button, history page and clear history button.

@phoenix7139 phoenix7139 changed the title Fixes #124 Implement Search history #124 Implement Search history Feb 11, 2019
@phoenix7139
Copy link
Author

phoenix7139 commented Feb 11, 2019

image1

User entering query into search bar

image2

query gets stored and displayed under history


document.addEventListener("DOMContentLoaded", function () {
document.querySelector("button").addEventListener("click", register);
document.querySelector("button").addEventListener("click",function()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a style linter.

Copy link
Author

Choose a reason for hiding this comment

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

Could you please elaborate? I don't know what a linter is

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sure. Linters help to find potential errors in code. You can look into eslint and prettier(style linter - not necessarily errors but helps to have standard coding style throughout the project).

Copy link
Author

Choose a reason for hiding this comment

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

I have corrected all the errors in the updated files in accordance with the Airbnb configuration of ESlint that was already setup. I have also added a max-len rule in the .eslintrc file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope you had fun learning about linters they have a pretty integral part in development.

I apologies so many changes might seem intimidating. But it is happening because this is a really big feature and is introducing a lot code into the codebase. Imagine if we straightaway merge this code, people who might want to contribute next year will face so much trouble to add more awesome features like these. Hope you understand.

Copy link
Author

Choose a reason for hiding this comment

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

Of course. This experience is teaching me a lot that I would probably haven't learnt otherwise

@PaliwalSparsh
Copy link
Collaborator

@phoenix7139 Great work with the PR and idea of feature. We will be going over few iterations and will make your PR ready to be added to the codebase. I have added few suggestions if you can incorporate them it will be really awesome.

popup/js/popup.js Outdated Show resolved Hide resolved
popup/js/popup.js Outdated Show resolved Hide resolved
options/js/options.js Outdated Show resolved Hide resolved
options/js/options.js Outdated Show resolved Hide resolved
aTag.setAttribute("href", recentSearchQueryUrls[count]);
historyListElement.textContent="";
var count=0;
recentSearchQueries.forEach(function(entr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

so the whole with making the array name as plural was we can write element name inside our functional loops as singular.
Ex - recentSearchQueries.forEach(function(query)..
So each element of recentSearchQueries is a query.

Always keep this thing in mind it is never never never bad to write big variable or function names (ex - popular repos have it (like react-redux.)[https://github.com/reduxjs/react-redux/blob/master/src/components/connectAdvanced.js#L168] have function names which are so self explanatory). They just need to as clear as possible. Its not an issue you will learn to do this slowly as you go through more and more codebases.

Also maybe (Airbnb style guide)[https://github.com/airbnb/javascript#the-javascript-style-guide-guide] has mentioned such tips in an awesome way. Ex - something as basic as making your boolean variable name start with is gives a lot of clarity to your code.

const isValid = true;
if (isValid) {
 // do this
}

options/js/options.js Outdated Show resolved Hide resolved
@phoenix7139
Copy link
Author

Is it okay to disable just a few eslint rules such as vars-on-top, no-redeclare and no-unused-vars?

Copy link
Member

@AvinashAgarwal14 AvinashAgarwal14 left a comment

Choose a reason for hiding this comment

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

Is it okay to disable just a few eslint rules such as vars-on-top, no-redeclare and no-unused-vars?

You can disable vars-on-top but don't disable no-redeclare and no-unused-vars. Also it would be great if you could squash recent 14 commits into a single commit.


function recordSearchHistory() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Amazing function name :) great work.

@@ -11,6 +11,11 @@
"no-var": "off",
"indent": ["error", 4],
"quotes": ["error", "double"],
"linebreak-style": "off"
"linebreak-style": "off",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think we should be removing any of these (Adding to warning is as good as removing people dont care about warnings). Max-len can be disabled at a place or two, but it is better to have it. It can be disabled you have a really really long string at some place.

Please can you share your motivation behind doing this. I am not able to find any reason as to why we are doing this ?

color: white;
background-color: grey;
padding: 8px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

tabs vs spaces :P
Please can you format this.


var radios = document.getElementsByName('theme');
if (!localStorage.getItem("theme")) localStorage.setItem("theme", "light");
Copy link
Collaborator

Choose a reason for hiding this comment

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

var theme = localStorage.getItem(THEME);
var isThemeNotSetInLocalStorage = !!theme; // think what could be a better name for the variable.
if ( isThemeNotSetInLocalStorage ) {
   localStorage.setItem(THEME, LIGHT);
}

Also we can move

var THEME = "theme";
var LIGHT = "light";
var DARK = "dark";

to the top of file and use these constants everywhere. It is better to move all constants to the top of file.

historyListElement.textContent = "";
count = 0;
recentSearchQueries.forEach(function (entr) {
var aTag = document.createElement("a");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you forgot to look into this. Please can you fix this variable name.

if (localStorage.getItem("search")) {
recentSearchQueries = JSON.parse(localStorage.getItem("search"));
}
var x = text.value;
Copy link
Collaborator

@PaliwalSparsh PaliwalSparsh Mar 6, 2019

Choose a reason for hiding this comment

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

Again what is x. Imagine someone coming to read your code and straightaway come to this line and tries to figure out what does x mean.

Also this is an perfect example of how important variable naming is. This text variable is being used throughout the file, it was declared in the top part of file. I had to go through the whole file to understand what this text variable points to. If it had a better name it would have been much easier to understand without scrolling hundereds of lines. Also, imagine tomorrow we have 2-3 more text fields. Now i cannot name all of them text (i hope you got the idea we wont be naming them text1 text2 text3 etc.). I would need to give all three understandable and readable names. While doing this I would need to rename this text variable and all places it is used at i.e. more work.

Most probably it would have been done by me only when i created this project. I learned all of this after looking into how other people code. So lets not fix this text var but you can fix your var name. I am planning to refactor the app a bit anyway so i will fix all these things then.

var recentSearchQueryUrls=[];
if(localStorage.getItem('link'))
recentSearchQueryUrls=JSON.parse(localStorage.getItem('link'));
var sPhrase="http://www.google.com/search?q="+query+" -"+uuid+" -inurl:(htm|html|php|pls|txt) intitle:index.of \"last modified\" ("+formats+")";
Copy link
Collaborator

Choose a reason for hiding this comment

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

sPhrase can have a better name.

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