Makes the back button work across tensorboard index (/) landing page #970

brianwa84 · 2018-02-12T20:08:16Z

Currently, if you come in via link to a tensorboard page on /, a plugin hash will immediately get appended (/#scalars). No amount of back button clicking will get you back to the linking page because the history item for / will get you immediately js-redirected back to /#scalars (or whatever).

This pull request allows us to specify that the storage state set call should be history-replacing (i.e. use 'window.location.replace(...)' instead of 'window.location.hash ='. Now when you link into a tensorboard server, you will be able to navigate back!

This allows the back button to work when you navigate to the index (/) URL.

…hod on strings!)

brianwa84 · 2018-02-12T20:09:24Z

I've followed the instructions here https://github.com/tensorflow/tensorboard/blob/master/DEVELOPMENT.md to verify this fix works locally.

nfelt · 2018-02-12T22:06:45Z

Thanks for this contribution! I'll take a look shortly.

Let user know which tags are missing per run (tensorflow#967)

chihuahua

I verified that this works as intended. When a user visits TensorBoard from a different web page, clicking back reverts to that web page. When a user switches dashboards, clicking back reverts to the previous dashboard.

Just have 1 nit.

chihuahua · 2018-02-14T22:46:05Z

tensorboard/components/tf_storage/storage.ts

-    window.location.hash = component;
+      if (useLocationReplace) {
+          window.location.replace(window.location.origin + window.location.pathname + '#' + component);
+      } else {


nit: After pathname, could you concatenate window.location.search?

Actually, you can just do: window.location.replace('#' + component);

Any preceding parts of the replacement URL not given will resolve against the current URL, including the query string. This is the same reason why you can use <a href="#top">Go to Top</a> and similar things inline in a page.

nfelt · 2018-02-14T22:57:44Z

tensorboard/components/tf_storage/storage.ts

-    window.location.hash = component;
+      if (useLocationReplace) {
+          window.location.replace(window.location.origin + window.location.pathname + '#' + component);
+      } else {


Actually, you can just do: window.location.replace('#' + component);

Any preceding parts of the replacement URL not given will resolve against the current URL, including the query string. This is the same reason why you can use <a href="#top">Go to Top</a> and similar things inline in a page.

nfelt · 2018-02-14T23:02:38Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -592,6 +592,7 @@ <h3>There’s no dashboard by the name of “<tt>[[_selectedDashboard]]</tt>.”
        if (activeDashboards && selectedDashboard == null) {
          selectedDashboard = activeDashboards[0] || null;
          if (selectedDashboard != null) {
+            tf_storage.setString(tf_storage.TAB, selectedDashboard, /*useLocalStorage=*/false, /*useLocationReplace=*/true);


Could you add a comment that explains why we're using location.replace here, i.e. to ensure that navigating back doesn't result in a loop? Also maybe note that the call to tf_storage.setString() in this case will supersede the one that will be invoked by _selectedDashboardChanged() once we update _selectedDashboard below.

Also, this line is a bit long, can you wrap it e.g. to

tf_storage.setString( tf_storage.TAB, selectedDashboard, /*useLocalStorage=*/false, /*useLocationReplace=*/true);

HTML files should follow 100 char limit.

jart

LGTM. Please note actionable items in other comments.

jart · 2018-02-15T11:56:28Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -592,6 +592,7 @@ <h3>There’s no dashboard by the name of “<tt>[[_selectedDashboard]]</tt>.”
        if (activeDashboards && selectedDashboard == null) {
          selectedDashboard = activeDashboards[0] || null;
          if (selectedDashboard != null) {
+            tf_storage.setString(tf_storage.TAB, selectedDashboard, /*useLocalStorage=*/false, /*useLocationReplace=*/true);


HTML files should follow 100 char limit.

brianwa84 · 2018-02-15T14:46:30Z

Not sure I totally understand the github workflow, I've made changes & committed them to my branch. Now what?

nfelt

LGTM, thanks again for fixing this!

Yep, you just make changes, commit to your branch, and push and they show up here for us to take another look.

nfelt · 2018-02-15T18:58:44Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -592,6 +592,11 @@ <h3>There’s no dashboard by the name of “<tt>[[_selectedDashboard]]</tt>.”
        if (activeDashboards && selectedDashboard == null) {
          selectedDashboard = activeDashboards[0] || null;
          if (selectedDashboard != null) {
+            // Use location.replace for this call to avoid breaking back-button navigation.
+            // Note that this will precede the update to tf_storage triggered by updating
+            // _selectedDashboard.


nit: maybe just add "... and make it a no-op" to the end of this comment for maximum clarity.

brianwa84 added 5 commits February 12, 2018 11:47

Extend storage.ts to allow using location.replace

fae994c

Use location.replace when selecting dashboard from active

25b7bb6

This allows the back button to work when you navigate to the index (/) URL.

Use location.hash.replace instead of deriving URL manually.

0218a94

Fix location replace (window.location.hash.replace is the replace met…

8835c2e

…hod on strings!)

pluginString not defined (want selectedDashboard)

95aacbc

nfelt self-assigned this Feb 12, 2018

nfelt self-requested a review February 12, 2018 22:03

Merge pull request #1 from tensorflow/master

fffbf43

Let user know which tags are missing per run (tensorflow#967)

chihuahua requested changes Feb 14, 2018

View reviewed changes

chihuahua added the core:frontend label Feb 14, 2018

nfelt reviewed Feb 14, 2018

View reviewed changes

jart approved these changes Feb 15, 2018

View reviewed changes

brianwa84 added 2 commits February 15, 2018 09:41

Simplify to use location.replace("#"+component)

8240b66

Add comment, reduce line length

aeaddd2

nfelt approved these changes Feb 15, 2018

View reviewed changes

Fix comment nit

92ec0ad

chihuahua approved these changes Feb 15, 2018

View reviewed changes

nfelt merged commit ab0ceb7 into tensorflow:master Feb 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Makes the back button work across tensorboard index (/) landing page #970

Makes the back button work across tensorboard index (/) landing page #970

Uh oh!

brianwa84 commented Feb 12, 2018

Uh oh!

brianwa84 commented Feb 12, 2018

Uh oh!

nfelt commented Feb 12, 2018

Uh oh!

chihuahua left a comment

Uh oh!

chihuahua Feb 14, 2018

Uh oh!

nfelt Feb 14, 2018

Uh oh!

nfelt Feb 14, 2018

Uh oh!

nfelt Feb 14, 2018

Uh oh!

jart Feb 15, 2018

Uh oh!

jart left a comment

Uh oh!

jart Feb 15, 2018

Uh oh!

brianwa84 commented Feb 15, 2018

Uh oh!

nfelt left a comment

Uh oh!

nfelt Feb 15, 2018

Uh oh!

Uh oh!

Makes the back button work across tensorboard index (/) landing page #970

Makes the back button work across tensorboard index (/) landing page #970

Uh oh!

Conversation

brianwa84 commented Feb 12, 2018

Uh oh!

brianwa84 commented Feb 12, 2018

Uh oh!

nfelt commented Feb 12, 2018

Uh oh!

chihuahua left a comment

Choose a reason for hiding this comment

Uh oh!

chihuahua Feb 14, 2018

Choose a reason for hiding this comment

Uh oh!

nfelt Feb 14, 2018

Choose a reason for hiding this comment

Uh oh!

nfelt Feb 14, 2018

Choose a reason for hiding this comment

Uh oh!

nfelt Feb 14, 2018

Choose a reason for hiding this comment

Uh oh!

jart Feb 15, 2018

Choose a reason for hiding this comment

Uh oh!

jart left a comment

Choose a reason for hiding this comment

Uh oh!

jart Feb 15, 2018

Choose a reason for hiding this comment

Uh oh!

brianwa84 commented Feb 15, 2018

Uh oh!

nfelt left a comment

Choose a reason for hiding this comment

Uh oh!

nfelt Feb 15, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!