Skip to content

color grouping: add group by regex under feature flag #5117

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

Merged
merged 2 commits into from
Jul 9, 2021

Conversation

japie1235813
Copy link
Contributor

@japie1235813 japie1235813 commented Jul 8, 2021

  • Motivation for features / changes
    We want to bring up color group by regex gradually by adding a feature flag enableColorGroupByRegex
    The feature flag is added in global (also shown in OSS) and we want to flip the flag in tb.corp only later (no concrete plan to bring color grouping in OSS yet)

  • Technical description of changes

    • show "Regex" under feature flag
    • the edit row is not yet added. thus click the regex won't do any effect (cause regexString is empty and no way to edit it now)
  • Screenshots of UI changes
    with ?enableColorGroupByRegex=true we can see third item on the dropdown. Click it should emit;
    Screen Shot 2021-07-08 at 4 30 53 PM

@google-cla google-cla bot added the cla: yes label Jul 8, 2021
Copy link
Contributor

@psybuzz psybuzz left a comment

Choose a reason for hiding this comment

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

Nice

(onGroupByChange)="onGroupByChange($event)"
></runs-group-menu-button-component>
`,
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class RunsGroupMenuButtonContainer {
showGroupByRegex$?: Observable<boolean> = this.store.select(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the '?' be dropped, since this field is initialized and never set to null/undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah you're right. thanks for catching this. removed

@japie1235813 japie1235813 merged commit af78290 into tensorflow:master Jul 9, 2021
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.

2 participants