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

Follow up issues on Shields adblock section in settings #23222

Closed
nullhook opened this issue Jun 2, 2022 · 4 comments · Fixed by brave/brave-core#13701
Closed

Follow up issues on Shields adblock section in settings #23222

nullhook opened this issue Jun 2, 2022 · 4 comments · Fixed by brave/brave-core#13701
Assignees
Labels
feature/shields/adblock Blocking ads & trackers with Shields OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-Win64 QA/Yes release-notes/exclude

Comments

@nullhook
Copy link

nullhook commented Jun 2, 2022

Fine-tune the Shield's Adblock settings frontend:

  • Add "Fade out" of filter lists at the bottom
  • First title of the page should be "Content filters" and not "Content Filtering"
  • Casing inconsistencies - "Custom Filters" -> "Custom filters"
  • "Custom lists" table section title should be 14px not 12px
  • Colors in the table:
    • Table heading color
    • Different row bg based on if the item is enabled
    • Red color for "Download failure"
  • Last updated should show time last updated even if latest try was failure
  • Subtitles get their different color style

Design

Screen Shot 2022-06-13 at 3 06 49 PM

Figma link: https://www.figma.com/file/tLXWGCpNoiJxDZDdpfordj/branch/A3RrnDB8Iib4k2ExACTNpM/Desktop-Settings

@nullhook nullhook added feature/shields/adblock Blocking ads & trackers with Shields OS/Desktop labels Jun 2, 2022
@nullhook nullhook self-assigned this Jun 2, 2022
@rebron rebron added priority/P3 The next thing for us to work on. It'll ride the trains. release-notes/exclude QA/Yes labels Jun 3, 2022
@antonok-edm
Copy link
Collaborator

Two more now that brave/brave-core#13421 is merged:

  • title field from the list data can be shown instead of the subscription URL, if available (and the column header should change from List URL to just Filter list)
  • homepage field from the list data can be presented as an additional link in the subscription's dropdown menu, if available

see older designs from #8107 for reference

@nullhook
Copy link
Author

nullhook commented Jun 7, 2022

Created a separate issue for the following as they're more invovled:

  1. "Reset to defaults" as it's more involved: Add "Reset to defaults" for adblock #23309
  2. Url validation Add URL validation in Adblock custom filters #23310
  3. Update and extend List URLs table #23341

@MadhaviSeelam
Copy link

MadhaviSeelam commented Jun 15, 2022

Verification PASSED using

Brave | 1.41.65 Chromium: 103.0.5060.42 (Official Build) nightly (64-bit)
-- | --
Revision | de0d840bf9439c31bd86bf74f065c31fdf9b208d-refs/branch-heads/5060@{#667}
OS | Windows 11 Version 21H2 (Build 22000.739)

Case 1:brave://adblock redirect to brave://settings/shields/filters - PASSED

  1. Install 1.41.65
  2. Launch brave
  3. Load brave://adblock
  4. Confirmed the redirect to brave://settings/shields/filters
  5. Confirmed brave://settings/shields/filters in a new tab loads successfully
  6. Also confirmed the UI matches to Figma screenshot in the PR
Step 4 Step 5&6
Step4 Step4

Case 2: Filter Lists link in the Shields panel redirects to brave://settings/shields/filters in NTP- PASSED

  1. Launch brave
  2. Load brave.com
  3. Click to open Shields panel
  4. Expand Advanced Controls
  5. Click Filter lists
  6. Confirmed brave://settings/shields/filters opens in NTP
Step5 Step6
Step4 Step4

@aguscruiz
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/shields/adblock Blocking ads & trackers with Shields OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-Win64 QA/Yes release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants