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 ability to remember previous search, and highlight search field… #43

Merged
merged 8 commits into from
Apr 23, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 13, 2017

… on popup open

tickets: #40 #37

Duplicate issues ^^

@ghost ghost added the feature This issue is a new feature. label Apr 13, 2017
@ghost ghost self-assigned this Apr 13, 2017
@ghost ghost requested a review from brandon1024 April 13, 2017 01:10
@brandon1024
Copy link
Owner

brandon1024 commented Apr 13, 2017

I've made a simplified version of popup.js, with some suggestions on ways we could keep the logic modular and open for features in the future, while keeping the logic concise and keeping separation of concerns. Let me know what you think :)

window.onload = function() {
    document.getElementById('searchField').addEventListener('keyup', updateLocalStorage);
    retrieveLastSearch(); //Add this here, once all the events are loaded and everything is gucci.
};

function updateLocalStorage() {
    //Create a JSON payload to send to local storage:
    var payload = {'key': 'value'};
    storeDataToLocalStorage(payload);
}

function storeDataToLocalStorage(payload) {
    //only a single method used to store data. Could also create a helper method to
    //update a value in storage.
    chrome.storage.local.set({payload: payload}, function callback() {
        //LOGGING
    });
}

function retrieveLastSearch() {
    chrome.storage.local.get('payload', function(data) {
        handleDataFromStorage(data)
    });
}

function handleDataFromStorage(data) {
    //Allows us to manage data in the future all in one location. Keeps this cohesive
    var storagePayload = data.payload;
    
    //grab data we want from payload...
    var previousSearchText = storagePayload.previousSearch;
    
    //do stuff with data...
    changeSearchFieldText(previousSearchText);
}

function changeSearchFieldText(text) {
    //updates to DOM should be done in a single method, to keep the 'view' stuff separate from
    //other logic stuff
    document.getElementById('searchField').value = text;
    document.getElementById("searchField").select();
}

//...

Note: This won't work as is. You would need to implement this into our current codebase.

@brandon1024
Copy link
Owner

Do you think we should be storing the previous search in the cloud rather than local storage? I've been thinking about it, and I think storing it locally would be a better option, and here is my reasoning:

If I had the extension installed on multiple computers (maybe one of which was a work pc) and happened to be signed into Chrome on both, it makes sense to keep the search history separate because keeping the previous query for your work on your work machine would be more useful.

If at home I were to look for, say, a name to find posts by a person in a Facebook feed, it wouldn't be useful to keep that at work.

What do you think?

@ghost
Copy link
Author

ghost commented Apr 13, 2017

@brandon1024 Yea sounds good. pretty sure its as simple as changing chrome.storage.sync to chrome.storage.local

@brandon1024
Copy link
Owner

I think so too, pretty sure its just a matter of changing sync to local.

@brandon1024
Copy link
Owner

Testing now 🔃

@brandon1024
Copy link
Owner

brandon1024 commented Apr 21, 2017

Testing Steps:

  • ✅ When opening the extension with no text in storage, loads up good:
    image

  • ✅ When opening the extension with text in storage, loads up, shows the previous search, and selects the text in the field:
    image

  • ✅ When changing the text from the previous search and closing the extension, on reopening the extension the new text is displayed:
    image

  • ❗ Using keyboard shortcut Command+Shift+F is overriden by the 'display fullscreen' command in Mac :( But using control+shift+f works good, so we can remove "mac": "Command+Shift+F" :)

@MikeWalz11 Changes look awesome, but looking into it a bit more I think we should update the local storage when the extension closes, not on every keypress.

Once this is fixed, I will merge it in 💯

Copy link
Owner

@brandon1024 brandon1024 left a comment

Choose a reason for hiding this comment

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

Once this is fixed, will merge :)

popup/popup.js Outdated
@@ -1,7 +1,7 @@

window.onload = function() {
document.getElementById('closeBtn').onclick = closeExtension;
document.getElementById('searchField').addEventListener('keyup', updateLocalStorage);
document.getElementById('searchField').addEventListener('input', updateLocalStorage);
Copy link
Owner

Choose a reason for hiding this comment

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

I believe we should change input to change. This stops it from updating the local storage with every added character. It also has all the correct behaviours, like when closing the extension everything saves correctly.

Copy link
Author

@ghost ghost Apr 22, 2017

Choose a reason for hiding this comment

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

@brandon1024 While testing I noticed that 'change' works well when closing application through close button, but when using keyboard shortcut the changes are not stored.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I see this now. Strange, it only had this behavior initially for me. Input event works better, so we may just need to sacrifice a bit of performance. Can update later if we find a solution. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants