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.

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

Use same labels on Shields global settings and panel #2149

Merged
merged 2 commits into from
Jun 12, 2019
Merged

Use same labels on Shields global settings and panel #2149

merged 2 commits into from
Jun 12, 2019

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Apr 2, 2019 •
edited
Loading

close brave/brave-browser#3589
close brave/brave-browser#3027

Screen Shot 2019-05-28 at 20 07 09

Test plan (extracted from :brave/brave-browser#3589 (comment))

In brave://settings/shields, text should be as follows:

In Shields Panel

- Cross-site trackers blocked [toggle]
- Connections upgraded to HTTPS [toggle]
- Scripts blocked [toggle]
- [ Cookies blocked / Cross-site cookies blocked / All cookies allowed ]
- [ Device recognition attempts blocked / Cross-site device recognition blocked / All device recognition attempts allowed ]

Change Shields global defaults [outlink]

In Settings

# Shields global defaults

- Block cross-site trackers [toggle]
- Upgrade connections to HTTPS [toggle]
- Block scripts [toggle]
- Cookies [ Block all cookies / Only block cross-site cookies / Allow all cookies ]
- Device recognition [ Block all device recognition attempts / Only block cross-site device recognition attempts / Allow all device recognition attempts ]

@cezaraugusto cezaraugusto self-assigned this Apr 2, 2019
@rossmoody rossmoody mentioned this pull request Apr 10, 2019
Merged
@cezaraugusto cezaraugusto force-pushed the ca-shields-settings branch from 3ff2ca7 to 32dd5eb Compare May 13, 2019 18:03
yrliou
yrliou reviewed May 13, 2019
View reviewed changes
app/brave_generated_resources.grd Outdated
<message name="IDS_SETTINGS_BRAVE_SHIELDS_HTTPS_EVERYWHERE_CONTROL_LABEL" desc="Default Brave HTTPS Everywhere control setting label">
HTTPS Everywhere
<message name="IDS_SETTINGS_BRAVE_SHIELDS_HTTPS_EVERYWHERE_CONTROL_LABEL" desc="Default Brave upgrade connections to HTTPS control setting label">
Upgrade connections to HTTPS
</message>
<message name="IDS_SETTINGS_BRAVE_SHIELDS_NO_SCRIPT_CONTROL_LABEL" desc="Default Brave script blocking control setting label">
Script blocking
Copy link
Member

Choose a reason for hiding this comment

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

According to the spec, this should be "Block scripts"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, updated!

yrliou
yrliou previously approved these changes May 13, 2019
View reviewed changes
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

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

Codes itself LGTM, please help make sure CI passes and get sign off from product/design for texts, thanks!

petemill
petemill reviewed May 13, 2019
View reviewed changes
app/brave_generated_resources.grd Outdated
</message>
<message name="IDS_SETTINGS_BRAVE_SHIELDS_DEFAULTS_DESCRIPTION" desc="The description for Brave shields Defaults sub-section in settings">
Your current per-site settings will be retained.
These are the default Shields settings for new sites. Changing these won't affect your existing per-site settings.
Copy link
Member

Choose a reason for hiding this comment

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

@tomlowenthal should we be using the word 'website' instead of 'site'? Also is it clear enough to say 'new sites'? It doesn't infer visiting that site explicitly and clearly. If you want suggestions, then how about something like "These are the default Shields settings when visiting websites. Changing these won't...."?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

"These are the default Shields settings. They apply to all websites unless you change something in the Shields panel on a particular site. Changing these won't affect your existing per-site settings."

@cezaraugusto cezaraugusto force-pushed the ca-shields-settings branch from 76051ef to 24f3d1d Compare May 24, 2019 14:32
@cezaraugusto
Copy link
Contributor Author

@tomlowenthal can you review the text content? I updated the screenshot

@tildelowengrimm
Copy link
Contributor

@cezaraugusto Could you add this explanatory text?

"These are the default Shields settings. They apply to all websites unless you change something in the Shields panel on a particular site. Changing these won't affect your existing per-site settings."

@cezaraugusto cezaraugusto force-pushed the ca-shields-settings branch from 24f3d1d to fecaa45 Compare May 28, 2019 23:07
@cezaraugusto
Copy link
Contributor Author

updated

@cezaraugusto cezaraugusto force-pushed the ca-shields-settings branch from fecaa45 to cef54be Compare June 5, 2019 00:40
@yrliou yrliou added this to the 0.68.x - Nightly milestone Jun 5, 2019
yrliou
yrliou previously approved these changes Jun 5, 2019
View reviewed changes
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

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

++ if CI passes

@bsclifton bsclifton force-pushed the ca-shields-settings branch from cef54be to 76e3310 Compare June 6, 2019 23:36
@cezaraugusto cezaraugusto force-pushed the ca-shields-settings branch 2 times, most recently from cef54be to 92381fb Compare June 7, 2019 19:28
@bsclifton bsclifton force-pushed the ca-shields-settings branch from 92381fb to 4e3763e Compare June 10, 2019 22:54
bsclifton
bsclifton reviewed Jun 11, 2019
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

CI running looked good, except for lint. I went ahead and added a commit cleaning up the lint 😄 Let's let this run and I think we'll be in good shape!

@bsclifton
Copy link
Member

Verified lint issues are fixed; however, seeing browser test failures on macOS:
https://staging.ci.brave.com/job/brave-browser-build-pr/job/ca-shields-settings/4/execution/node/391/log/

Going to pull down the code locally to verify...

@bsclifton
Copy link
Member

bsclifton commented Jun 11, 2019 •
edited
Loading

Getting failures testing locally... going to try replaying the browser test using CI (which will do a fresh init, etc):
https://staging.ci.brave.com/job/brave-browser-build-pr/job/ca-shields-settings/5/

@bsclifton bsclifton added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Jun 11, 2019
bsclifton
bsclifton approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Everything passed! 👍

@bsclifton bsclifton merged commit d1b3a08 into master Jun 12, 2019
@bsclifton bsclifton deleted the ca-shields-settings branch June 12, 2019 06:17
@tildelowengrimm tildelowengrimm mentioned this pull request Jun 12, 2019
Closed
@agentofuser agentofuser mentioned this pull request Jun 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petemill petemill petemill left review comments

@bsclifton bsclifton bsclifton approved these changes

@yrliou yrliou yrliou left review comments

@tildelowengrimm tildelowengrimm Awaiting requested review from tildelowengrimm

Assignees

@cezaraugusto cezaraugusto

Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS feature/shields
Projects
None yet
Milestone
0.68.x - Release
5 participants
@cezaraugusto @tildelowengrimm @bsclifton @petemill @yrliou