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

Only display active dashboards #181

Merged
merged 1 commit into from
Jul 7, 2017
Merged

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Jul 5, 2017

Summary:
This commit incorporates the backend's is_active plugin functions and
the related plugins_listing handler, and uses them on the client: we
will only display dashboards that correspond to backend plugins that are
active.

Test Plan:

  • Launch TensorBoard with a normal data set and note that the correct
    set of dashboards is displayed.
  • Launch TensorBoard with a nonexistent or empty logdir and observe
    the global "no data" message.
  • Launch TensorBoard and cause the plugins_listing network request
    to fail. (In Chrome: Network devtools tab, right-click the request,
    select "Block Request URL", refresh.) Note the resulting message.
    Then, unblock the request, and click TensorBoard's reload button:
    everything should work as usual.
  • Launch TensorBoard with a large data set, and open the frontend
    before it's read any text summaries. Then, click TensorBoard's
    reload button and note that the "Text" dashboard tab appears and
    functions.
  • Launch TensorBoard on a large data set. Select the "text" dashboard.
    Then, relaunch the TensorBoard backend on the same port, and click
    the frontend's reload button before any text summaries are loaded.
    Note that TensorBoard removes the text dashboard and shoves the user
    over to a different dashboard (the first dashboard loaded) with a
    notice.
  • Launch TensorBoard, let it load data, then kill the server and click
    TensorBoard's reload button. Note that the already-loaded features
    of TensorBoard are still operational.

wchargin-branch: active-dashboards

Copy link
Contributor

@teamdandelion teamdandelion left a comment

Choose a reason for hiding this comment

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

Very nice! Some changes requested:

  • Keep a list of inactive dashboards, as a dropdown on the right ('5 inactive dashboards') and let user explicitly load it if they want.
  • If user puts hash for inactive dashboard, nav to it anyway.
  • Make font consistent for the "loading dashboards" message in top bar.
  • If we keep the toast, it should be on top not bottom of window.
  • Fix the broken logdir test.

@wchargin wchargin force-pushed the wchargin-fix-text-is-active branch from 80fafcc to 81212a6 Compare July 5, 2017 22:31
@wchargin
Copy link
Contributor Author

wchargin commented Jul 5, 2017

Thanks for the review! I'll post a comment here when I've addressed each of these.

  • Keep a list of inactive dashboards, as a dropdown on the right ('5 inactive dashboards') and let user explicitly load it if they want.
  • If user puts hash for inactive dashboard, nav to it anyway.
  • Make font consistent for the "loading dashboards" message in top bar.
  • If we keep the toast, it should be on top not bottom of window.
  • Fix the broken logdir test.

@wchargin wchargin changed the base branch from wchargin-fix-text-is-active to master July 6, 2017 01:04
@wchargin wchargin force-pushed the wchargin-active-dashboards branch from 3eb3c5e to cf3873d Compare July 6, 2017 01:04
@wchargin
Copy link
Contributor Author

wchargin commented Jul 6, 2017

Okay! Changes summarized below.

I've removed the toast; you can now navigate to a nonexistent plugin and you'll receive a standard warning:

Screenshot: "There's no dashboard by the name of dashboard-name."

Inactive dashboards are presented in a separate dropdown and may be selected:

Screenshot: The "inactive dashboards" dropdown when an inactive dashboard is selected.

Screenshot: The "inactive dashboards" dropdown when an active dashboard is selected.

If you're focused on an inactive dashboard, then you use TensorBoard's reload button, and the dashboard becomes active, then you'll still be focused on it as it moves to the list of active dashboards.

The logdir is shown when there is a warning message, which fixes the test.
Screenshot: The logdir is shown at the bottom of the warning message.

@wchargin wchargin force-pushed the wchargin-active-dashboards branch from cf3873d to e68eba6 Compare July 6, 2017 19:44
@wchargin
Copy link
Contributor Author

wchargin commented Jul 6, 2017

Per your request, I've noinked the dropdown. (Personally, I prefer it with ink because of the immediate feedback; the UI feels higher-latency to me without the ink. But I'm fine with using noink.)

The focus state on the dropdown items is required for a11y.

PTAL :-)

@teamdandelion
Copy link
Contributor

Discussed offline:

  • Some reformatting of the "inactive dashboards" dropdown. It would be nice if it were totally cohesive with the tabs.
  • Let's add some WebDriver testing :)

@wchargin wchargin force-pushed the wchargin-active-dashboards branch from e68eba6 to 66fd107 Compare July 7, 2017 02:01
@wchargin
Copy link
Contributor Author

wchargin commented Jul 7, 2017

Enjoy! I've included a WebDriver test that I think is reasonably comprehensive. (Let's see what Travis thinks…)

Active dashboard selected

Inactive dashboard selected

Summary:
This commit incorporates the backend's `is_active` plugin functions and
the related `plugins_listing` handler, and uses them on the client: we
will only display dashboards that correspond to backend plugins that are
active.

Test Plan:
  - Launch TensorBoard with a normal data set and note that the correct
    set of dashboards is displayed.
  - Launch TensorBoard with a nonexistent or empty logdir and observe
    the global "no data" message.
  - Launch TensorBoard and cause the `plugins_listing` network request
    to fail. (In Chrome: Network devtools tab, right-click the request,
    select "Block Request URL", refresh.) Note the resulting message.
    Then, unblock the request, and click TensorBoard's reload button:
    everything should work as usual.
  - Launch TensorBoard with a large data set, and open the frontend
    before it's read any text summaries. Then, click TensorBoard's
    reload button and note that the "Text" dashboard tab appears and
    functions.
  - Launch TensorBoard on a large data set. Select the "text" dashboard.
    Then, relaunch the TensorBoard backend on the same port, and click
    the frontend's reload button before any text summaries are loaded.
    Note that TensorBoard removes the text dashboard and shoves the user
    over to a different dashboard (the first dashboard loaded) with a
    notice.
  - Launch TensorBoard, let it load data, then kill the server and click
    TensorBoard's reload button. Note that the already-loaded features
    of TensorBoard are still operational.

Also, `bazel run //tensorboard/functionaltests:core_test` and step away
from your computer for a minute or so.

wchargin-branch: active-dashboards
@wchargin wchargin force-pushed the wchargin-active-dashboards branch from 66fd107 to a0a939f Compare July 7, 2017 20:56
@wchargin
Copy link
Contributor Author

wchargin commented Jul 7, 2017

(Last push was just a rebase; no content change.)

@wchargin wchargin merged commit d25eacb into master Jul 7, 2017
@wchargin wchargin deleted the wchargin-active-dashboards branch July 11, 2017 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants