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

Patch 3 #15

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

Patch 3 #15

wants to merge 2 commits into from

Conversation

userlinksgopher
Copy link

Added filtering on text
Added extraction of links from selection
Added message "No links"
Updated locale ru
Added filteringDomains (addNodes)

user and others added 2 commits December 18, 2017 17:51
Added extraction of links from selection
Added message "No links"
Updated locale ru
Added filteringDomains (addNodes)
@userlinksgopher
Copy link
Author

Hi, please reply. Patch will be merged into the master branch?

@az0
Copy link
Owner

az0 commented Dec 20, 2017

Thank you for the code. I would like a little time to review. When the review is all done and any issues are resolved, I will merge it into the master branch.

(In general it is easier for me to review smaller chunks, such as one feature or one bug fix per pull request, but overall this looks like not a large change.)

@az0
Copy link
Owner

az0 commented Dec 21, 2017

I tested the new functionality in Firefox 57, and the first three features work well. I started a new mini-site with testing pages. For example, there is a simple page without links.

Would you please elaborate on filteringDomains?

Also, would you please make two changes?

First, it searches now by either the link URL or link text, but the user prompt is no longer accurate.

Enter a string of characters to search for within the link. Links without this string will be ignored.

How about this?

Enter a string of characters to search for within the link address or link text. Links without this string will be ignored.

This would be easy for me to change, but you would need to update the translation too.

Second, searching by selection may not be intuitive because it not stated in the user interface. If the user highlights a portion, he may be surprised not all links are returned. If the user wants only a portion of links to be returned, he may not know that highlighting enables this functionality.

Therefore, I propose to add a checkbox in popup.html called Search in selection.

The default state of the checkbox is either

  1. Off
  2. Remembered (stored in the configuration)
  3. On if and only if there is an active selection in the current tab

@userlinksgopher
Copy link
Author

Would you please elaborate on filteringDomains?

I knew that this question will be! :) That's my fault...
Of course, the string "Added filteringDomains (addNodes)" should be removed from commit.
In fact, in the function addNodes //filteringDomains to be replaced by //domains.
I left it there that would know where the string or object is sent to the function.

First, it searches now by either the link URL or link text, but the user prompt is no longer accurate.

Enter a string of characters to search for within the link. Links without this string will be ignored.

How about this?

Enter a string of characters to search for within the link address or link text. Links without this string will be ignored.

This would be easy for me to change, but you would need to update the translation too.

I agree. It can be done

Second, searching by selection may not be intuitive because it not stated in the user interface. If the user highlights a portion, he may be surprised not all links are returned. If the user wants only a portion of links to be returned, he may not know that highlighting enables this functionality.

Therefore, I propose to add a checkbox in popup.html called Search in selection.

The default state of the checkbox is either

  1. Off
  2. Remembered (stored in the configuration)
  3. On if and only if there is an active selection in the current tab

Create extra controls...
Maybe change the text on the button when there is a selection?
If I understand correctly, it is necessary from popup.js determine whether there is a selection.
But I don't know how to do... Immediately on https://developer.mozilla.org not found.
Where to find the answer? Need help, please :)

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.

2 participants