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.

Sign up for GitHub

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

Jump to bottom

Updates to shields panel v2 #12523

Merged

nullhook merged 1 commit into master from shields_follow_ups

Mar 15, 2022

Contributor

nullhook commented Mar 8, 2022 •

edited

Loading

Added favicon observer
Added "Report site" window
Added web component button
Modified a specified text color on light theme
Don't trigger shields bubble on internal scheme pages
Overflow scroll to auto
Copy fixes
External links should work
Added pref boolean check to show/hide block count on icon
Added tooltip on the new shields icon

Resolves brave/brave-browser#21416
Resolves brave/brave-browser#21559
Resolves brave/brave-browser#21572
Resolves brave/brave-browser#21570
Resolves brave/brave-browser#21583
Resolves brave/brave-browser#21613
Resolves brave/brave-browser#17369
Resolves brave/brave-browser#21581

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:

github-actions bot added the CI/storybook-url label

github-actions bot assigned nullhook

nullhook force-pushed the shields_follow_ups branch from 9789f0b to f0f9939 Compare

March 8, 2022 10:00

nullhook requested a review from simonhong

March 8, 2022 10:01

Member

simonhong commented Mar 9, 2022

Need to check unit test & storybook failures.

[2022-03-08T10:33:21.066Z]     BraveShieldsDataControllerTest.GetAdBlockMode_ForOrigin (../../brave/browser/ui/brave_shields_data_controller_unittest.cc:299)
[2022-03-08T10:33:21.066Z]     BraveShieldsDataControllerTest.SetAdBlockMode_ForOrigin_1 (../../brave/browser/ui/brave_shields_data_controller_unittest.cc:107)
[2022-03-08T10:33:21.066Z]     BraveShieldsDataControllerTest.SetAdBlockMode_ForOrigin_2 (../../brave/browser/ui/brave_shields_data_controller_unittest.cc:171)
[2022-03-08T10:33:21.066Z]     BraveShieldsDataControllerTest.SetAdBlockMode_ForOrigin_3 (../../brave/browser/ui/brave_shields_data_controller_unittest.cc:235)

nullhook force-pushed the shields_follow_ups branch from f0f9939 to 8c705b8 Compare

March 9, 2022 23:42

Contributor Author

nullhook commented Mar 9, 2022

@simonhong Fixed all crashes

nullhook force-pushed the shields_follow_ups branch from 8c705b8 to 54c0665 Compare

March 9, 2022 23:54

Collaborator

brave-builds commented Mar 10, 2022

A Storybook has been deployed to preview UI for the latest push

simonhong reviewed

View reviewed changes

browser/ui/brave_shields_data_controller.cc Outdated Show resolved Hide resolved

browser/ui/brave_shields_data_controller_unittest.cc Outdated Show resolved Hide resolved

nullhook force-pushed the shields_follow_ups branch from 54c0665 to b6e3447 Compare

March 10, 2022 04:05

nullhook requested a review from simonhong

March 10, 2022 04:07

Collaborator

brave-builds commented Mar 10, 2022

A Storybook has been deployed to preview UI for the latest push

simonhong reviewed

View reviewed changes

components/brave_shields/resources/panel/stories/component-panel.tsx Show resolved Hide resolved

nullhook force-pushed the shields_follow_ups branch from b6e3447 to 780c5e9 Compare

March 10, 2022 09:25

Collaborator

brave-builds commented Mar 10, 2022

A Storybook has been deployed to preview UI for the latest push

nullhook force-pushed the shields_follow_ups branch from 780c5e9 to cd8bc74 Compare

March 10, 2022 17:40

Collaborator

brave-builds commented Mar 10, 2022

A Storybook has been deployed to preview UI for the latest push

simonhong reviewed

View reviewed changes

browser/ui/views/brave_actions/brave_shields_action_view.cc Outdated Show resolved Hide resolved

nullhook force-pushed the shields_follow_ups branch from cd8bc74 to 77a928c Compare

March 11, 2022 02:28

Collaborator

brave-builds commented Mar 11, 2022

A Storybook has been deployed to preview UI for the latest push

nullhook mentioned this pull request

Shields v2: Some texts need a change brave/brave-browser#21583

Closed

nullhook force-pushed the shields_follow_ups branch from 77a928c to 9137e74 Compare

March 11, 2022 03:34

nullhook requested review from simonhong and petemill

March 11, 2022 03:35

simonhong reviewed

View reviewed changes

browser/ui/views/brave_actions/brave_shields_action_view.cc Outdated Show resolved Hide resolved

Collaborator

brave-builds commented Mar 11, 2022

A Storybook has been deployed to preview UI for the latest push

simonhong approved these changes

View reviewed changes

Member

simonhong left a comment

Don't have more comments for c++ side.
I'll pass webui changes review to @petemill

test/BUILD.gn Outdated Show resolved Hide resolved

browser/ui/views/brave_actions/brave_shields_action_view.cc Outdated Show resolved Hide resolved

nullhook force-pushed the shields_follow_ups branch from babfa96 to 5fa7aef Compare

March 11, 2022 22:51

Collaborator

brave-builds commented Mar 11, 2022

A Storybook has been deployed to preview UI for the latest push

nullhook mentioned this pull request

Brave Shields: enable v2 UI refresh by default and introduce flag for legacy v1 #12586

Merged

25 tasks

nullhook force-pushed the shields_follow_ups branch from 5fa7aef to 1d4744f Compare

March 14, 2022 02:36

nullhook commented

View reviewed changes

browser/ui/views/brave_actions/brave_shields_action_view.cc Outdated Show resolved Hide resolved

Collaborator

brave-builds commented Mar 14, 2022

A Storybook has been deployed to preview UI for the latest push

Tonev mentioned this pull request

Shields v2: No text/tooltip on hovering on the Shields icon brave/brave-browser#21581

Closed

nullhook force-pushed the shields_follow_ups branch 2 times, most recently from 5a5d554 to d21fef2 Compare

March 14, 2022 08:34

Collaborator

brave-builds commented Mar 14, 2022

A Storybook has been deployed to preview UI for the latest push

simonhong reviewed

View reviewed changes

browser/ui/views/brave_actions/brave_shields_action_view.cc Outdated Show resolved Hide resolved


          Updates to shields panel v2

3876d05

- Added favicon observer
- Added web component button

nullhook force-pushed the shields_follow_ups branch from d39a5a5 to 3876d05 Compare

March 15, 2022 00:48

Collaborator

brave-builds commented Mar 15, 2022

A Storybook has been deployed to preview UI for the latest push

petemill approved these changes

View reviewed changes

Member

petemill left a comment

nice polish 👍

Just a couple nits or follow-ups

components/brave_shields/resources/panel/components/tree-list/index.tsx

@@ @@ -64,14 +70,13 @@ function TreeList (props: Props) { @@
                       </div>
                     </S.TreeBox>
                     <S.Footer>
-                      <S.BackButton
-                        type="button"
+                      <Button
                         aria-label="Back to previous screen"

Member

petemill Mar 15, 2022

follow-up: should be translated

components/resources/brave_shields_strings.grdp

@@ @@ -18,7 +18,7 @@ @@
                 <message name="IDS_BRAVE_SHIELDS_BROKEN" desc="A footnote below the main Shields switch button.">
                   If this site seems broken, try Shields down.
-                  <ph name="LINK_BEFORE">$1</ph>Note: this may reduce Brave privacy protections.<ph name="LINK_AFTER">$2</ph>
+                  <ph name="LINK_BEFORE">$1</ph>Note: this may reduce Brave's privacy protections.<ph name="LINK_AFTER">$2</ph>

Member

petemill Mar 15, 2022

This isn't really a link is it? Just styling differently for the second line? Also I'm not sure how translations will handle or preserve the line-break (and whether we should rely on that anyway).
I suggest we split in to 2 strings to be simpler.

Member

petemill commented Mar 15, 2022

Also it feels to me like the buttons which link to the detailed tree lists should have a cursor: pointer since the rest of our buttons seem to

nullhook merged commit cb120f1 into master

nullhook deleted the shields_follow_ups branch

March 15, 2022 08:10

Contributor Author

nullhook commented Mar 15, 2022

merged, will do follow ups on a separate PR

nullhook added this to the 1.38.x - Nightly milestone

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