Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1631)

Unified Diff: chrome/browser/resources/settings/settings_menu/settings_menu.js

Issue 2184893002: Settings Router Refactor: Remove route.page legacy property. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@0217-settings-refactor-settings-menu
Patch Set: update test Created 4 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/resources/settings/settings_menu/settings_menu.js
diff --git a/chrome/browser/resources/settings/settings_menu/settings_menu.js b/chrome/browser/resources/settings/settings_menu/settings_menu.js
index 9047a049442fab62cbbc7d6a0351ad7379045cd8..5b20dd4accf783211c8be9fadc41cf9bdcccc6df 100644
--- a/chrome/browser/resources/settings/settings_menu/settings_menu.js
+++ b/chrome/browser/resources/settings/settings_menu/settings_menu.js
@@ -48,7 +48,8 @@ Polymer({
this.fire('toggle-advanced-page', false);
}.bind(this));
- this.fire('toggle-advanced-page', this.currentRoute.page == 'advanced');
+ this.fire('toggle-advanced-page',
+ settings.Route.ADVANCED.contains(this.currentRoute));
},
/**
@@ -57,19 +58,10 @@ Polymer({
*/
currentRouteChanged_: function(newRoute) {
// Sync URL changes to the side nav menu.
-
- if (newRoute.page == 'advanced') {
+ this.$.advancedMenu.selected = this.currentRoute.path;
michaelpg 2016/07/29 20:28:53 if this works, can we just use (one-way) data bind
tommycli 2016/07/29 21:04:40 Done. Nice
michaelpg 2016/07/29 21:32:56 Awesome, we got rid of an entire currentRouteChang
+ this.$.basicMenu.selected = this.currentRoute.path;
assert(!this.pageVisibility ||
this.pageVisibility.advancedSettings !== false);
- this.$.advancedMenu.selected = this.currentRoute.section;
- this.$.basicMenu.selected = null;
- } else if (newRoute.page == 'basic') {
- this.$.advancedMenu.selected = null;
- this.$.basicMenu.selected = this.currentRoute.section;
- } else {
- this.$.advancedMenu.selected = null;
- this.$.basicMenu.selected = null;
- }
},
/**
@@ -92,22 +84,10 @@ Polymer({
*/
openPage_: function(event) {
this.ripple_(/** @type {!Node} */(event.currentTarget));
- var page = event.currentTarget.parentNode.dataset.page;
- if (!page)
- return;
-
- var section = event.currentTarget.dataset.section;
- // TODO(tommycli): Use Object.values once Closure compilation supports it.
- var matchingKey = Object.keys(settings.Route).find(function(key) {
- var route = settings.Route[key];
- // TODO(tommycli): Remove usage of page, section, and subpage properties.
- // Routes should be somehow directly bound to the menu elements.
- return route.page == page && route.section == section &&
- route.subpage.length == 0;
- });
- if (matchingKey)
- settings.navigateTo(settings.Route[matchingKey]);
+ var route = settings.getRouteForPath(event.currentTarget.dataset.path);
michaelpg 2016/07/29 20:28:53 unfortunate that the menu has to do this -- settin
tommycli 2016/07/29 21:04:40 Yeah I know. Ideally I'd prefer to do settings.na
+ assert(route, 'settings-menu has an an entry with an invalid path');
+ settings.navigateTo(route);
},
/**

Powered by Google App Engine
This is Rietveld 408576698