Skip to content

Conversation

@psybuzz
Copy link
Contributor

@psybuzz psybuzz commented Nov 20, 2019

  • Motivation for features / changes
    Add the ability for embedders to inject items in the header, next to the 'Reload', 'Settings' buttons.

  • Technical description of changes

    • Adds a new slot for extra header items, to the left of the 'Reload' button, centered vertically
    • Drive by fixes the slot name for 'injected-overview'. Slots define insertion map by the 'name' attribute, not 'id'
    • Re-enable the tf-tensorboard test

@wchargin
Copy link
Contributor

Cancelled build to unblock #2947, without which this build would fail
anyway.

});

it('reloads the active dashboard on request', (done) => {
xit('reloads the active dashboard on request', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer to accompany skipped tests with a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

xdescribe, too. if you think they are stale, maybe just omit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - added to xdescribe

checkSlottedUnderAncestor(headerItem1, '.header');
checkSlottedUnderAncestor(headerItem2, '.header');

function checkSlottedUnderAncestor(
Copy link
Contributor

Choose a reason for hiding this comment

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

several higher level comments:

  1. hoist this up function since it is general utility.
  2. Polymer.dom($0).deepContains($1) may want to look into this.
  3. consider moving new additions in tf_util to testing.ts or tf_util/shadow_dom.ts since we probably want some common utilities for ShadowDom that are missing from Polymer.dom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Done
  2. Thanks, I didn't know Polymer offered this!
  3. I'll keep in mind for the future. Polymer's deepContains() addresses the need here, but I imagine we might want our own Shadow utils in the future

@psybuzz psybuzz requested a review from stephanwlee November 20, 2019 16:29

describe('top right global icons', function() {
// TODO(psybuzz): Restore or remove these tests. This folder's tests seems
// to have not been running since the GitHub repo history was tracked.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: instead of describing the history, for TODOs at least, describe precondition that can resolve this TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewritten to note that the test was not running, and at least 1 reason for their failure.

@psybuzz psybuzz closed this Nov 22, 2019
@psybuzz psybuzz reopened this Nov 22, 2019
@psybuzz psybuzz merged commit df135f5 into tensorflow:master Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants