Persisted advanced controls area in Shields v2 #13985
Conversation
github-actions
bot
added
the
CI/storybook-url
| @@ -43,3 +47,18 @@ void ShieldsPanelHandler::GetPosition(GetPositionCallback callback) { | |||
| brave_browser_window_->GetShieldsBubbleRect().y()); | |||
| std::move(callback).Run(vec); | |||
| } | |||
|
|
|||
| void ShieldsPanelHandler::SetAdvancedViewEnabled(bool is_enabled) { | |||
| DCHECK(profile_); | |||
There was a problem hiding this comment.
even though we have a check in an upper layer it doesn't hurt to have one here.
|
A Storybook has been deployed to preview UI for the latest push |
There was a problem hiding this comment.
Do you think we need to address the case where I open shields in 1 window, then open it in another window and expand advanced controls. When I return to shields in the first window the advanced controls will be collapsed? We'd need to either
- observe the pref changed in the c++ and call a JS callback when it changes, or
- fetch the pref when window visibility changes
What do you think @nullhook?
| const toggleIsExpanded = () => { | ||
| setIsExpanded(prevState => { | ||
| const newState = !prevState | ||
| getPanelBrowserAPI().panelHandler.setAdvancedViewEnabled(newState) |
There was a problem hiding this comment.
Should this be inside the callback to compute the state value? Seems weird. Possible performance impact though doesn't seem different to doing it outside the function.
On a similar note, do you even need the function, since toggleIsExpanded isn't memo-ized it'll always have access to the current value.
const newValue = !isExpanded
setIsExpanded(newValue)
getPanelBrowserAPI().panelHandler.setAdvancedViewEnabled(newValue)There was a problem hiding this comment.
The state updater function accesses it's own previous state so I don't see any weirdness? but I do get what you did with closure. To keep it simple, i'll move the logic outside of state updater function.
nullhook
force-pushed
the
shields-btn-expand
branch
from
0293c1e to
86482e9
Compare
Good catch. I've added a visibility observer to solve this. |
|
A Storybook has been deployed to preview UI for the latest push |
nullhook
force-pushed
the
shields-btn-expand
branch
from
86482e9 to
dce511b
Compare
| @@ -1,5 +1,6 @@ | |||
| import * as React from 'react' | |||
| import getPanelBrowserAPI, { UIHandlerReceiver, SiteBlockInfo, SiteSettings } from '../api/panel_browser_api' | |||
| import { loadTimeData } from '../../../../common/loadTimeData' | |||
There was a problem hiding this comment.
note that you can use '$web-common/loadTimeData' for this path
|
A Storybook has been deployed to preview UI for the latest push |
Resolves brave/brave-browser#23188
This PR utilizes the existing pref which was used on previous Shields version to keep the advanced controls area expanded.
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
After you've expanded once, you should be able to see the Advanced controls expanded by default in the following use cases: