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

[Beta] new log query UI feedback #1828

Closed
emlimap opened this issue Jun 23, 2020 · 14 comments
Closed

[Beta] new log query UI feedback #1828

emlimap opened this issue Jun 23, 2020 · 14 comments
Assignees
Milestone

Comments

@emlimap
Copy link

emlimap commented Jun 23, 2020

I have been testing the new UI in master branch and have some feedback. UI was tested on iOS 13.5 Safari

  1. The unblock button at the bottom of query detailed view pop-up looks like a heading rather than a button since it uses same font size as other headings. It would be better if it was presented as a button to makes it obvious that it is a button.

  2. On blocked requests, the rule that caused the request to be blocked and the list that contains the rule are no longer displayed. Not sure if it is intentional as current stable version provides this information.

  3. Can we make the search field not case-sensitive. Mobile keyboards default to capitalizing first letter of first word and search returns no results when first letter is capitalized. For example, Crashlytics would return no results but crashlytics would.

  4. When you tap on search field, as the OS brings up the keyboard it also zooms a bit which causes the UI to overflow a bit outside the screen. For example half of close button in query detailed view popup overflow outside the screen.

Screenshot:

IMG_1665

@twcau
Copy link

twcau commented Jun 30, 2020

Especially concur on points 3 and 4. This is a real frustration.

@emlimap
Copy link
Author

emlimap commented Jul 6, 2020

@ArtemBaskal Regarding point 3. I don't see it being addressed as part of the commit, which makes me believe that the removal of information regarding what filter and list caused the block was intentional. Can you confirm?

@ameshkov
Copy link
Member

ameshkov commented Jul 6, 2020

@emlimap why, it is addressed in this commit actually: 21dfb5f

@ameshkov
Copy link
Member

ameshkov commented Jul 6, 2020

@ArtemBaskal point 2 is not addressed.

@ameshkov ameshkov reopened this Jul 6, 2020
@emlimap
Copy link
Author

emlimap commented Jul 6, 2020

@ameshkov Oops. Meant to say point 2. Hence, why I was wondering if that was intentional as part of redesign or not. It would be good to keep it as if you want to try to work out what filter & list caused the block, it is a lot more work on mobile to do so.

@adguard adguard closed this as completed in 36f3218 Jul 6, 2020
@emlimap
Copy link
Author

emlimap commented Jul 8, 2020

@ArtemBaskal Thank you surfacing the info regarding the list responsible for block as well fixing the UI overflow bug in mobile.

Apologies for discussing on a closed issue but one more feedback on point 2. Feel free to let me know if this should be a new issue and I can open one

I think the new UI makes it less clear why something is blocked when it is blocked by a CNAME.

In the current stable release, 0.102, it makes it obvious why securemetrics.apple.com is blocked as well as the filter responsible for it. All the info in one page.
image

The same query in new UI is less clearer as to why it was blocked but does point you in the right direction by saying which list caused the block.
image

If you then check the domain name, securemetrics.apple.com using inbuilt checker, it will respond with the domain name not being in any of your list, including default Adguard list. This is because the checker doesn't check for CNAME or IP blocks by resolving the DNS but rather just checking if the domain name exists in any of the lists subscribed.

image

Obviously you can work this out by other ways but they involve a lot more work for something the current release makes it easy.

My suggestion would be, instead of saying Blocked in status, it should say Blocked by CNAME or IP response for blocked caused by CNAME or IP. Looks like we already have this info and should surface it as it helps keeping the user informed. We could also surface the filter responsible for the block as well.

@ameshkov
Copy link
Member

ameshkov commented Jul 8, 2020

@emlimap first of all, the fact that it does not show the rule text is a bug

Second, I suppose we should instead improve "Check the filtering" form and either allow entering additional details, or make it query it by itself. I even think we already have an open issue about that.

@ameshkov ameshkov reopened this Jul 8, 2020
@ameshkov
Copy link
Member

ameshkov commented Jul 8, 2020

@ArtemBaskal

Also, we can extend the tooltip with the information on the original answer.
Check this screenshot: https://uploads.adguard.com/up04_Screenshot_o8319.png

IF: The status is Blocked AND the "original_answer" field is present
DO:
Change the status to Blocked by CNAME or IP
Add "Original response" field to the response details

How to reproduce with a local AdGuard Home instance:

  1. Add appleglobal.102.112.2o7.net to the custom rules
  2. Disable other blocklists
  3. Run nslookup securemetrics.apple.com 127.0.0.1

@ameshkov
Copy link
Member

ameshkov commented Jul 8, 2020

@ArtemBaskal

And one more thing. Let's get rid of empty-value lines in all details tooltips.
If the value is not set - just don't print that line at all.

image

@emlimap
Copy link
Author

emlimap commented Jul 8, 2020

@ameshkov Had a quick search in Github issues but couldn't find a feature request related to adding CNAME or IP block check support to Check the filtering.

I think the idea of being able to have a link to check why something is blocked by providing a link that directly taking the user to filter checker and populating the domain name from blocked query is a good improvement. Especially for mobile users.

CNAME support in filter checker combined with this feature request #1734 to add block and unblock buttons would massively improve the user experience.

I am not sure if direct link to filter checker is needed anymore now that the info filter checker will provider is added back into tooltip along with unblock button.

@temporallyaccount
Copy link

temporallyaccount commented Jul 10, 2020

We dont have below part on new UI and it's more important

image

@emlimap
Copy link
Author

emlimap commented Jul 13, 2020

@ameshkov Hopefully this is the last time I have to comment on closed issue.

In a previous comment #1828 (comment) you asked empty fields to be hidden in popup UI. Does this apply to Mobile UI as well? Still seeing couple of empty fields and I think mobile UI would stand to benefit the most as a lot of information is crammed into a small screen and you don't have to scroll far to block/unblock.

Some of the empty fields under Client details can be hidden. Also not sure what is with empty Response field. On Desktop I not am seeing a field called Response but can see Response code with NOERROR.

Screenshot:

IMG_1683

Those empty fields are hidden on desktop view though. We just need to extend it to mobile UI popup as well. Small nitpick, long IP owner names could be handled better as at the moment it overflows which is fine but the word gets cut by half.

image

@ameshkov
Copy link
Member

ameshkov commented Jul 13, 2020

Does this apply to Mobile UI as well?

Yeah, we should fix this.

Some of the empty fields under Client details can be hidden. Also not sure what is with empty Response field. On Desktop I not am seeing a field called Response but can see Response code with NOERROR.

Response code must be present in the mobile UI as well

Small nitpick, long IP owner names could be handled better as at the moment it overflows which is fine but the word gets cut by half.

Agreed, they need not be wrapped.

One more issue:

And one more with too long hostnames:

@ArtemBaskal ask @IldarKamalov help with the layout.

And one more -- there's no "Response" for requests filtered by Safe search:

Screenshot

image

@ArtemBaskal the general rule is very simple -- if either "original response" or "response" field is not empty, you should print "Original Response" or "Response" data.

@ameshkov ameshkov reopened this Jul 13, 2020
@emlimap
Copy link
Author

emlimap commented Jul 15, 2020

@ameshkov @ArtemBaskal The latest changes has addressed all the pending issue and the UI looks perfect imho. All the information you need at quick glance but can drill down further into a query using tool tips. Also hiding empty fields made it more cleaner. Thank you both for the work on updated query UI.

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

No branches or pull requests

5 participants