Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236723002:
MD Settings: Hide the advanced page toggle control during searching. (Closed)
|
Created:
4 years, 4 months ago by dpapad Modified:
4 years, 4 months ago Reviewers:
Dan Beam CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Hide the advanced page toggle control during searching.
Also ensure that after the user clears the search results, the advanced page
returns to the expanded/collapsed state it had before searching.
BUG=629696
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/c72fd9a779aaf29554c3ba4e08ff58602eff54d8
Cr-Commit-Position: refs/heads/master@{#411429}
Patch Set 1 : Fix #
Total comments: 8
Patch Set 2 : Remove showingSearchResults_, re-use previousShowPages instead. #
Total comments: 4
Patch Set 3 : Address comments. #
Messages
Total messages: 24 (14 generated)
Description was changed from ========== MD Settings: Hide the advanced page toggle control during searching. Also ensure that after the user clears the search results, the advanced page returns to the expanded/collapsed state it had before searching. BUG=629696 ========== to ========== MD Settings: Hide the advanced page toggle control during searching. Also ensure that after the user clears the search results, the advanced page returns to the expanded/collapsed state it had before searching. BUG=629696 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
lgtm https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:46: return {about: false, basic: false, advanced: false}; can we just add the advanced toggle to this? https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:76: showingSearchResults_: { why do we need this if we can just check for previousShowPages_ existing? https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:139: !this.showingSearchResults_; why is this part of the check needed? https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:222: this.previousShowPages_ = this.showPages_; if this is available in the same scope, why do we need to bind to |this.|?
doh, didn't mean to lg not lgtm
https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:76: showingSearchResults_: { On 2016/08/10 at 23:17:41, Dan Beam wrote: > why do we need this if we can just check for previousShowPages_ existing? I am using showingSearchResults as a boolean var from the HTML. Perhaps we could get away with |prevousShowPages_| only, but my initial thinking is that it will be more cryptic to have |previousShowPages_| serve a dual purpose. For example the data binding in HTML will look like <template is="dom-if" if="[[showAdvancedToggle_(showPages_.basic, inSubpage_, previousShowPages_)]]"> I'll try merging those vars and upload in a new patch, so we can see if it is better. https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:139: !this.showingSearchResults_; On 2016/08/10 at 23:17:41, Dan Beam wrote: > why is this part of the check needed? This change causes the advanced toggle to be hidden when the page is displaying search results, per the request of @tbuckley at https://bugs.chromium.org/p/chromium/issues/detail?id=629696#c7. https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:222: this.previousShowPages_ = this.showPages_; On 2016/08/10 at 23:17:41, Dan Beam wrote: > if this is available in the same scope, why do we need to bind to |this.|? We need to cache the previousShowPages on this, because it is used during different invocations of searchContents(). Explaining further below. 1) user nagigates to chrome://md-settigns. 2) user invokes search 'foo', line 222 executes. Lines 257-258 do not execute yet, because we are displaying the results of 'foo'. 3) user clicks on the 'x' icon in the search box to clear the results. searchContents('') is called. line 222 is skipped because we are still showingSearchResults_. After the SearchManager#search() resolves, line 257-258 executes because we are no longer showing search results. Does that make sense? I would be happy to explain further if that is still confusing.
https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2236723002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:76: showingSearchResults_: { > I'll try merging those vars and upload in a new patch, so we can see if it is better. Done in latest patch.
lgtm https://codereview.chromium.org/2236723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2236723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:54: * @type {?{about: boolean, basic: boolean, advanced: boolean}} can we typedef this? https://codereview.chromium.org/2236723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:130: showAdvancedToggle_: function() { nit: var isSearching = !!this.previousShowPages_; return this.showPages_.basic && !this.inSubpage_ && !isSearching;
https://codereview.chromium.org/2236723002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2236723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:54: * @type {?{about: boolean, basic: boolean, advanced: boolean}} On 2016/08/11 at 05:32:34, Dan Beam wrote: > can we typedef this? Done. https://codereview.chromium.org/2236723002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/settings_main/settings_main.js:130: showAdvancedToggle_: function() { On 2016/08/11 at 05:32:34, Dan Beam wrote: > nit: > > var isSearching = !!this.previousShowPages_; > return this.showPages_.basic && !this.inSubpage_ && !isSearching; Done. I chose a slightly different name, inSearchMode, because isSearching can be misleading here, it can be confused with "is search in progress", which is what toolbarSpinnerActive depicts, which is different than inSearchMode.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2236723002/#ps100001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Hide the advanced page toggle control during searching. Also ensure that after the user clears the search results, the advanced page returns to the expanded/collapsed state it had before searching. BUG=629696 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Hide the advanced page toggle control during searching. Also ensure that after the user clears the search results, the advanced page returns to the expanded/collapsed state it had before searching. BUG=629696 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/c72fd9a779aaf29554c3ba4e08ff58602eff54d8 Cr-Commit-Position: refs/heads/master@{#411429} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c72fd9a779aaf29554c3ba4e08ff58602eff54d8 Cr-Commit-Position: refs/heads/master@{#411429} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
