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

step selector: add control to settings panel behind feature flag #5734

Merged
merged 3 commits into from
Jun 6, 2022

Conversation

japie1235813
Copy link
Contributor

@japie1235813 japie1235813 commented Jun 2, 2022

  • Motivation for features / changes
    This is part of the work to separate the "sticky data table" (aka step selector) to its own feature. This PR adds the checkbox to enable/disable step selector. The checkbox is hided behind feature flag (enableScalarDataTable).

  • Technical description of changes
    Add visual component to settings panel and wiring toggle action. (no dispatch action yet)

TODO: add ngrx state to control enabling time selector and plump them together
TODO: consider renaming feature flag

  • Screenshots of UI changes
    Screen Shot 2022-06-02 at 2 03 51 PM

@japie1235813 japie1235813 changed the title time selector: add control to settings panel behind feature flag step selector: add control to settings panel behind feature flag Jun 2, 2022
@japie1235813 japie1235813 requested a review from JamesHollyer June 2, 2022 22:44
Copy link
Contributor

@JamesHollyer JamesHollyer left a comment

Choose a reason for hiding this comment

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

LGTM. I made a suggestion for another test.

@@ -88,6 +88,7 @@ describe('metrics right_pane', () => {
store.overrideSelector(selectors.getIsFeatureFlagsLoaded, true);
store.overrideSelector(selectors.getIsMetricsImageSupportEnabled, true);
store.overrideSelector(selectors.getIsLinkedTimeEnabled, false);
store.overrideSelector(selectors.getIsDataTableEnabled, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see a test that shows it does not render when this is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Added.

@japie1235813 japie1235813 merged commit db79ff2 into tensorflow:master Jun 6, 2022
japie1235813 added a commit that referenced this pull request Jun 7, 2022
This is part of the work to wiring the "sticky data table" (aka step selector) to its own feature. Following up #5734, we create the ngrx state and toggle to change the state status, which will be used to turn on/off the feature.
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
…sorflow#5734)

This is part of the work to separate the "sticky data table" (aka step selector) to its own feature. This PR adds the checkbox to enable/disable step selector. The checkbox is hided behind feature flag (enableScalarDataTable).
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
…5742)

This is part of the work to wiring the "sticky data table" (aka step selector) to its own feature. Following up tensorflow#5734, we create the ngrx state and toggle to change the state status, which will be used to turn on/off the feature.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
…sorflow#5734)

This is part of the work to separate the "sticky data table" (aka step selector) to its own feature. This PR adds the checkbox to enable/disable step selector. The checkbox is hided behind feature flag (enableScalarDataTable).
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
…5742)

This is part of the work to wiring the "sticky data table" (aka step selector) to its own feature. Following up tensorflow#5734, we create the ngrx state and toggle to change the state status, which will be used to turn on/off the feature.
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.

2 participants