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.

Sign up for GitHub

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

MDCMigration: Restyle all plugins selector component for MDC migration. #6612

Merged
merged 3 commits into from
Oct 3, 2023
Merged

MDCMigration: Restyle all plugins selector component for MDC migration. #6612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b846aae
Revert "MDCMigration: Restyle the header tabs (#6606)"
bmd3k Sep 29, 2023
663297e
Style plugin_selector_component for Angular MDC Component migration.
bmd3k Oct 2, 2023
78c5f89
Fix chevron coloring.
bmd3k Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions tensorboard/webapp/header/plugin_selector_component.ng.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@
limitations under the License.
-->
<mat-tab-group
mat-stretch-tabs="false"
class="active-plugin-list"
[selectedIndex]="getActivePluginIndex()"
animationDuration="100ms"
>
<mat-tab *ngFor="let plugin of activePlugins">
<mat-tab *ngFor="let plugin of activePlugins" [disabled]="!plugin.enabled">
Copy link
Contributor

Choose a reason for hiding this comment

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

We had some weird bugs around this disabled property before. However, this seems to all be working great.

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 just kept the code that was originally there before we made the branch.

<ng-template mat-tab-label>
<!-- Manually subscribe to the click event on the tab content element.
Cannot trust the selectedTabChange event since it is async and can cause
Expand All @@ -36,12 +37,13 @@
</mat-tab-group>
<mat-form-field
floatLabel="never"
*ngIf="disabledPlugins.length"
*ngIf="disabledPlugins.length > 0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just checking length is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably but this is the code that was there before we made the branch so I prefer not to change it.

subscriptSizing="dynamic"
>
<mat-label>Inactive</mat-label>
<mat-select
[value]="selectedPlugin"
[hideSingleSelectionIndicator]="true"
(selectionChange)="onDisabledPluginSelectionChanged($event)"
>
<mat-option
Expand Down
181 changes: 144 additions & 37 deletions tensorboard/webapp/header/plugin_selector_component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,59 +14,166 @@ limitations under the License.
==============================================================================*/
@import 'tensorboard/webapp/theme/tb_theme';

@mixin plugin-text {
color: map-get($tb-dark-foreground, text);
font-weight: 500;
text-transform: uppercase;
}

:host {
align-items: center;
display: flex;
flex: 1 1 auto;
font-size: 14px;
height: 100%;
justify-content: space-between;
overflow: hidden;
}

// Mat tabs use the theme primary color as their text color.
mat-tab-group ::ng-deep {
.mat-mdc-tab {
padding: 0 8px;
margin: 0 16px;
height: 64px;
:host mat-form-field ::ng-deep {
// Default width is calculated by the contents of the longest value in the
// select. We override both the trigger and panel widths to be a bit shorter
// than what we see in practice.
// TODO: In Angular 16+ use the panelWidth attribute to set the width of the
// panel to be different than the trigger. (We would likely want the
// trigger to be shorter and the panel to be longer).
width: 144px;

.mdc-tab-indicator__content {
border-color: currentColor;
}
.mdc-text-field {
@include tb-theme-background-prop(background-color, app-bar);
// Default padding is "0 16px".
padding: 0 4px;
}

label.mdc-floating-label.mat-mdc-floating-label,
.mat-mdc-select,
.mat-mdc-select-value,
.mat-mdc-select-arrow {
// Inherit from `color` on the toolbar.
color: inherit;
}

.mdc-text-field--filled:not(.mdc-text-field--disabled)
.mdc-line-ripple::before {
// Inherit from `border-color` on the toolbar.
border-color: inherit;
}
}

mat-label,
mat-select,
mat-option {
font-size: 14px;
font-weight: 500;
text-transform: uppercase;
}

.active-plugin-list {
align-self: stretch;
flex: 1 1 auto;
overflow: hidden;
}

.plugin-name {
align-items: center;
display: inline-flex;
height: 100%;
justify-content: center;
padding: 0 12px;
width: 100%;
}

:host ::ng-deep .active-plugin-list {
// Override mat-tab styling. By default, mat-tab has the right styling but,
// here, we are using it under dark header background. Must invert the color.

.mat-mdc-tab:not(.mat-mdc-tab-disabled)
.mdc-tab-indicator__content--underline {
border-color: currentColor;
}

.mat-mdc-tab:not(.mat-mdc-tab-disabled) {
.mdc-tab__text-label {
// Inherit from `color` on the toolbar.
color: inherit;
// default is .6 and it is too dark against dark background.
opacity: 0.7;
}

&.mdc-tab--active .mdc-tab__text-label {
// Repeat color with more-specific selector to override dark-mode styling.
// Inherit from `color` on the toolbar.
color: inherit;
opacity: 1;
}
}

.mat-mdc-tab-header-pagination {
color: inherit;
}

.mat-mdc-tab-header-pagination-chevron {
border-color: currentColor;
}

.mat-mdc-tab-header-pagination-disabled {
visibility: hidden;
}

.mat-mdc-tab-disabled {
display: none;
}

mat-form-field ::ng-deep {
// There are only two appearance options
// 1) "filled" - with a background
// 2) "outline" - with a border
// Both will require a restyle like this. I'm opting for filled
// because it is the default.
.mdc-text-field--filled {
background: none;
mat-mdc-tab-list,
.mat-mdc-tab-header,
.mat-mdc-tab-labels,
// Extra-specific selector to override dark-mode styling.
.mat-mdc-tab-header .mat-mdc-tab,
.mdc-tab__text-label {
height: 100%;
}

.mat-mdc-tab {
letter-spacing: 0.25px;
min-width: 48px; /* default is 90px which is too big for us */
padding: 0; /* default is 24px */
text-transform: uppercase;
}

mat-tab-header {
.mat-mdc-tab-list {
// 36px is the size of the chevron. Please see [1] for the reason.
padding: 0 36px;
}

// The bottom border of the select menu.
.mdc-text-field--filled {
::before {
border-bottom-color: map-get($tb-dark-foreground, text);
> {
:first-child,
.mat-mdc-tab-label-container,
:last-child {
// [1]: Reason for customizing the mat-tab-header.
//
// Default mat-tab only renders the directional overflow chevron when
// width of the label container is smaller than mat-tab-header. This
// causes visual jank when user resizes the screen as the mat-tab with
// the chevron appears to have more padding (visually; directional
// chevron can have `visibility: hidden` in case it is not needed and
// appear as padding). To have the same experience as the Polymer based
// Material tab header, we always set the padding of 36px on each sides
// but that causes the scroll calculation to be incorrect and causes a
// bug [2].
// To work around it, we make everything `position: absolute`.
// [2]: https://github.com/tensorflow/tensorboard/issues/4841
bottom: 0;
position: absolute;
top: 0;
}

:first-child,
.mat-mdc-tab-label-container {
left: 0;
}
}

mat-label,
.mat-mdc-select-arrow {
@include plugin-text;
:last-child,
.mat-mdc-tab-label-container {
right: 0;
}

.mat-mdc-tab-header-pagination {
Copy link
Contributor

Choose a reason for hiding this comment

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

The color of the pagination carrot is black in light mode which looks strange. I think it should stay the same color in both light and dark mode just like the plugin name text.

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.

@include tb-theme-background-prop(background-color, app-bar);
}
}
}
}

mat-option,
.plugin-name {
@include plugin-text;
}
9 changes: 2 additions & 7 deletions tensorboard/webapp/header/plugin_selector_container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@ const getUiPlugins = createSelector(getPlugins, (listing): UiPluginMetadata[] =>
Object.keys(listing).map((key) => Object.assign({}, {id: key}, listing[key]))
);

const getActivePlugins = createSelector(
getUiPlugins,
(plugins): UiPluginMetadata[] => plugins.filter((plugin) => plugin.enabled)
);

const getDisabledPlugins = createSelector(
getUiPlugins,
(plugins): UiPluginMetadata[] => plugins.filter((plugin) => !plugin.enabled)
Expand All @@ -37,7 +32,7 @@ const getDisabledPlugins = createSelector(
selector: 'plugin-selector',
template: `
<plugin-selector-component
[activePlugins]="activePlugins$ | async"
[activePlugins]="plugins$ | async"
[disabledPlugins]="disabledPlugins$ | async"
[selectedPlugin]="activePlugin$ | async"
(onPluginSelectionChanged)="onPluginSelectionChange($event)"
Expand All @@ -46,7 +41,7 @@ const getDisabledPlugins = createSelector(
})
export class PluginSelectorContainer {
readonly activePlugin$ = this.store.pipe(select(getActivePlugin));
readonly activePlugins$ = this.store.pipe(select(getActivePlugins));
readonly plugins$ = this.store.pipe(select(getUiPlugins));
readonly disabledPlugins$ = this.store.pipe(select(getDisabledPlugins));

constructor(private readonly store: Store<State>) {}
Expand Down