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

Moved Adblock page to settings #12827

Merged
merged 1 commit into from
Jun 1, 2022
Merged

Moved Adblock page to settings #12827

merged 1 commit into from
Jun 1, 2022

Conversation

nullhook
Copy link
Contributor

@nullhook nullhook commented Mar 30, 2022

Resolves brave/brave-browser#8838

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Go to: brave://settings/shields/content-filters

  1. Content filtering
    Toggle on/off items, refresh the page, and see if your selection/s persist
    Type keywords that includes in the list and ensure the list if the display is being filtered or not. "Show full list" button should hide in filtered state.

  2. Add Custom Filter Lists
    Test url: https://raw.githubusercontent.com/DandelionSprout/adfilt/master/Anti-'Custom%20cursors'%20List.txt

  • Update the list, you will notice "Last updated" text change
  • Remove the list
  • Test with a non-filter url (google.com), you should be able to see "Download failure" as it's not a valid list
  1. Custom Filters
    Add CSS selectors and test if they're being blocked

@nullhook nullhook changed the title Adblock page (WIP DO NOT MERGE) Moved Adblock page to settings (WIP DO NOT MERGE) Mar 30, 2022
@nullhook nullhook force-pushed the adblock_page_new branch 2 times, most recently from 3f99c4e to dd43dcf Compare April 4, 2022 10:41
@nullhook nullhook force-pushed the adblock_page_new branch 4 times, most recently from 5d52d3e to 39db460 Compare April 8, 2022 16:46
@nullhook nullhook force-pushed the adblock_page_new branch 2 times, most recently from 251c036 to ddfc1f3 Compare April 22, 2022 21:29
@nullhook nullhook changed the title Moved Adblock page to settings (WIP DO NOT MERGE) Moved Adblock page to settings Apr 22, 2022
@nullhook nullhook force-pushed the adblock_page_new branch 3 times, most recently from 9be7c2d to 5ab41d9 Compare April 24, 2022 20:18
@nullhook nullhook requested a review from a team as a code owner April 25, 2022 15:12
@nullhook nullhook requested a review from petemill April 25, 2022 15:15
@nullhook nullhook force-pushed the adblock_page_new branch 3 times, most recently from ca02829 to a3fc8d9 Compare May 4, 2022 19:15
@nullhook nullhook requested a review from goodov May 9, 2022 20:27
@petemill petemill force-pushed the adblock_page_new branch from a3fc8d9 to 1f9a690 Compare May 16, 2022 22:05
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

chromium_src lgtm

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

This is really good - an advanced UI in polymer nicely done with clean logic, binding and styles.

It could have been nice to see the commits separated in to 2 - one for the refactor to move from html imports to es imports for the existing code, and the other to introduce the new filter list subpage. But that's ok since it's still somewhat manageable to review because of the file separation.

I only have minor feedback I think.

edit: I also tested out all 3 sections of features and all the sub-features worked well!

@nullhook nullhook force-pushed the adblock_page_new branch 2 times, most recently from c62f906 to f869a5c Compare May 20, 2022 04:43
@nullhook nullhook requested a review from petemill May 20, 2022 04:46
@antonok-edm antonok-edm mentioned this pull request May 20, 2022
25 tasks
@@ -113,3 +113,18 @@
</defs>
</svg>
</iron-iconset-svg>
<iron-iconset-svg name="brave_lion" size="16">
Copy link
Member

Choose a reason for hiding this comment

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

This is promising but I'm confused about a couple of things:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is suppose to be brave_leo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@nullhook nullhook force-pushed the adblock_page_new branch from f869a5c to 798342c Compare May 24, 2022 00:57
@nullhook nullhook requested a review from petemill May 24, 2022 00:59
@nullhook nullhook force-pushed the adblock_page_new branch from 798342c to 925e48a Compare May 24, 2022 01:18
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Some design issues when I compare to the figma:

  • "Filter lists" search input is missing the search icon. cr-input seems to support a suffix slot, which may be what you want. Actually, probably inline-suffix.

  • Missing the link on "Annoyances"

  • Missing the "Brave's tracking filters" title and subtitle content
    image

  • Heading titles are 16px in the design but 13.65px in the code

  • Spacing seems off after first subtitle due to double margin (container and paragraph). Same for before the "Enter url" input in the custom filter lists section, and the textarea in the Custom filters section
    image

  • Custom filter lists should have a title, a subtitle and a note. Looks like you've combined the subtitle and the note to one item, missing the new paragraph and the different styling for the note.

  • Missing the "Custom lists" table title

  • Subscribe menu icons don't seem vertical-center-aligned to the text

image

Other issues (that can come with follow-up if you prefer):

  • "Fade out" of filter lists at the bottom is not implemented.
    image
  • Missing "Reset to defaults" button. Do we have an API for that, or would we need to create it?
  • Colors in custom lists aren't added:
    • Table heading color
    • Different row bg based on if the item is enabled
    • Red color for "Download failure"
      image
      image

Note that the change to Poppins is coming in this PR so I wouldn't neccessarily focus on that, but if you wanted to make sure any custom text not part of components (i.e. not buttons) matched the design, you could put font-family: Poppins in your custom filters subpage.

@nullhook nullhook force-pushed the adblock_page_new branch from 925e48a to 7697b46 Compare May 25, 2022 01:22
@nullhook nullhook requested a review from petemill May 25, 2022 01:22
@nullhook nullhook force-pushed the adblock_page_new branch 2 times, most recently from 637fff5 to 97cb916 Compare May 25, 2022 05:29
@petemill
Copy link
Member

I'm still seeing an issue with icon alignment.
image
I think the intention of the Leo icons in figma is for them to be centered inside their viewbox, and they are placed that way, but doesn't seem to be coming out that way. Could we try to solve that? Perhaps we are copying the svg from the incorrect outer element in figma? Just a guess.

I also see a few of the other items of feedback aren't addressed yet. @nullhook please could you post a comment mentioning which issues should be fixed and which aren't for whatever reason?

@nullhook
Copy link
Contributor Author

nullhook commented May 26, 2022

The following issues are not addressed in this PR and will be resolved separately:

  1. "Fade out" of filter lists at the bottom is not implemented
  2. "Reset to defaults" - need to create an API
  3. Colors in the table - Figma doesn't have dark theme equivalents of the table colors yet
  4. Status color - needs to be well thought out as there are two other states

The path in svg should be centered to the viewBox now:

image

@nullhook nullhook force-pushed the adblock_page_new branch from 97cb916 to f0dc2e2 Compare May 26, 2022 19:13
@@ -990,6 +990,7 @@
<part file="wallet_strings.grdp" />
<part file="tor_strings.grdp" />
<part file="brave_shields_strings.grdp" />
<part file="brave_adblock_strings.grdp" />
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed this, but strings for the settings page go in app/brave_settings_strings.grdp. You can probably just move the grdp and reference it from that. This is not a resource for components (and also shouldn't be included on android or ios).

Copy link
Contributor Author

@nullhook nullhook May 31, 2022

Choose a reason for hiding this comment

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

moved adblocked related strings to app/brave_settings_strings

Copy link
Member

@petemill petemill 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, but please move the strings and have CI pass before merge.

Let's ensure we action or create follow-up issue(s) for the following:

  • Subtitles get their different color style
    image
  • 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
  • Table colors
  • "Download failed" error color
  • Last updated should show time last updated even if latest try was failure
    image
  • Basic url validation when clicking "Add" on custom filter lists input

@nullhook nullhook force-pushed the adblock_page_new branch 2 times, most recently from 2eea596 to 8892782 Compare June 1, 2022 05:27
@nullhook nullhook force-pushed the adblock_page_new branch from 8892782 to 4c5b27a Compare June 1, 2022 19:08
@nullhook nullhook merged commit b50949b into master Jun 1, 2022
@nullhook nullhook deleted the adblock_page_new branch June 1, 2022 22:02
@nullhook nullhook added this to the 1.41.x - Nightly milestone Jun 1, 2022
@nullhook
Copy link
Contributor Author

nullhook commented Jun 2, 2022

Follow up issues thread created: brave/brave-browser#23222

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.

move brave://adblock/ to brave://settings/shields
3 participants