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

Tweaks to the query log filters to make them a bit more user friendly #1568

Closed
wants to merge 6 commits into from

Conversation

PromoFaux
Copy link
Member

@PromoFaux PromoFaux commented Aug 28, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

  • Allows mobile users to be able to use the new filtering options as well as desktop users
  • Removes need to hold down a modifier key to add or remove to/from the filter. Simply click the field to toggle it on/off
  • Utilises the ctrl/meta modifier keys for desktop users to copy domain to the clipboard (mobile users can long-press the domain and use native browser functionality to copy highlighted text)

How does this PR accomplish the above?:

What documentation changes (if any) are needed to support this PR?:
Possibly docs, I've not checked. This may need to be replicated to long term tables?

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux PromoFaux marked this pull request as draft August 28, 2020 07:42
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/how-am-i-supposed-to-filter-the-query-logs-with-a-mobile-phone-browser/37656/12

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Address prettier complaints

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux
Copy link
Member Author

mobile functionality:

ezgif-2-1a6e4ecc4645

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux
Copy link
Member Author

Desktop functionality (didn't have time to make a gif last night!
filters

@yubiuser
Copy link
Member

yubiuser commented Oct 11, 2020

  1. The tooltip (mouse over) still recommends the old behavior
  2. The text selection (Ctrl+mouse) does not work on my linux + firefox. (But does work on Linux+ Chrome)
  3. I never see the showAlert after copying

wait, something is wrong here...

@yubiuser
Copy link
Member

I fixed it.

The only thing that remains:

  1. The tooltip (mouse over) still recommends the old behavior

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux
Copy link
Member Author

PromoFaux commented Oct 11, 2020

Ah, OK.. what was happening with ff on linux?

I have sorted the tooltip text

image

image

@yubiuser
Copy link
Member

Ah, OK.. what was happening with ff on linux?

It was always selecting the whole cell. I figured out that the local repository was in a weird state, I nuked it and cloned it again. Now it is working as expected.

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Looks good to me.

document.body.appendChild(tmpTextarea);
tmpTextarea.value = text;
tmpTextarea.select();
document.execCommand("copy");
Copy link
Member

@DL6ER DL6ER Oct 11, 2020

Choose a reason for hiding this comment

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

Thanks for your suggestion. However, I've just found

This feature [document.execCommand] is obsolete. Although it may still work in some browsers, its use is discouraged since it could be removed at any time. Try to avoid using it.

https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand

and

The execCommand('copy') API isn't supported in Safari

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Interact_with_the_clipboard

There seems to be the new asynchronous Clipboard API methods to add this feature, however, ...

The asynchronous Clipboard API methods are a recent addition to the specification, and may not be fully implemented to the specification in all browsers. Be sure to review the compatibility tables for each method before using them, to ensure that support is broad enough for your needs.

(same source as above)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Fiddlesticks...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look at using this newer API. Maybe it's time we say "to hell with internet explorer", anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, the clipboard API is still in working draft status. So probably best not to use it....

@PromoFaux
Copy link
Member Author

Killing this. It's got messy and needed rebasing anyway...

@PromoFaux PromoFaux closed this Oct 12, 2020
@PromoFaux
Copy link
Member Author

Redone as #1602

@PromoFaux PromoFaux deleted the tweak/queryfilters branch October 12, 2020 16:45
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.

None yet

4 participants