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

Add support for multiple views on Shields #2874

Merged
merged 6 commits into from Jul 10, 2019
Merged

Add support for multiple views on Shields #2874

merged 6 commits into from Jul 10, 2019

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jul 5, 2019

close brave/brave-browser#1196
close brave/brave-browser#4953

shields-views

see https://brave-ui-4200l5i5w.now.sh/?path=/story/feature-components-shields--panel

Test Plan

See brave/brave-browser#1196 for design spec

  1. Initial state for Shields should be the "simple view" (no option for controlling resources blocked)
  2. First time switching to "advanced view" should show a warning. This warning should show only. one time, i.e. toggling views again should not show it.
  3. Should be able to toggle simple/advanced views via link in Shields footer.

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

Copy link

@karenkliu karenkliu left a comment

Hi @cezaraugusto! This looks good, but I was wondering if there was any way we could fit in these last minute small changes to this PR:

simple view QA 3

If not, could we please get this in for the next build?

cezaraugusto added 5 commits Jul 9, 2019
close brave/brave-browser#4953

Shields state needed across windows/tabs is not stored in localStorage API.
This follows the webUI convention for persistent state.

Goal is to provide a way to handle upcoming changes such as onboarding screen
and simple/advanced view setting.

Other state such as stats, resources blocked, and blocking preferences are still
handled by the back-end and are left untouched.
@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Jul 9, 2019

@karenkliu updated

@@ -121,7 +121,7 @@ describe('cosmeticFilterReducer', () => {
shieldsPanelReducer(initialState.shieldsPanel, {
type: windowTypes.WINDOW_REMOVED,
windowId
})
})
Copy link
Member

@petemill petemill Jul 10, 2019

Choose a reason for hiding this comment

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

nit: whitespace

Copy link
Contributor Author

@cezaraugusto cezaraugusto Jul 10, 2019

Choose a reason for hiding this comment

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

updated

@@ -196,6 +226,9 @@ describe('shieldsPanelState test', () => {
it('updates the currentWindowId', () => {
expect(shieldsPanelState.updateFocusedWindow(state, 2)).toEqual({
currentWindowId: 2,
persistentData: {
Copy link
Member

@petemill petemill Jul 10, 2019

Choose a reason for hiding this comment

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

nit: this all seems quite repetitive! Perhaps a function getDefaultState would remove the need to copy and keep in sync amongst the tests, next time we need to update this :-)

Copy link
Contributor Author

@cezaraugusto cezaraugusto Jul 10, 2019

Choose a reason for hiding this comment

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

are you ok if we address this in a follow-up? I've been looking to re-work some shields tests for a while, can add this as well

Copy link
Member

@petemill petemill Jul 10, 2019

Choose a reason for hiding this comment

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

Absolutely, it was just a comment

Copy link
Member

@bsclifton bsclifton left a comment

Comments left about spelling - pulled down locally, works great! 😄 I made sure to try toggling the views, going to the learn more view, and also quitting/re-opening/browsing other domains to make sure view choice was retained

Great work here! 😄 👍

@cezaraugusto cezaraugusto merged commit 51b2a37 into master Jul 10, 2019
1 of 2 checks passed
@cezaraugusto cezaraugusto deleted the ca-1196 branch Jul 10, 2019
@cezaraugusto cezaraugusto added this to the 0.69.x - Nightly milestone Jul 10, 2019
@cezaraugusto cezaraugusto mentioned this pull request Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment